[9front] [patch] skelfs: remove useless code

[9front] [patch] skelfs: remove useless code

[Arne Meyer]

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

Hello,
  
if I read the code correctly emalloc9p already checks for failed allocations and zeroes the memory, so the following code is redundant.

Greetings,
Arne

[-- Attachment #2: skelfs.patch --]
[-- Type: application/octet-stream, Size: 336 bytes --]

diff e5d29a2bd91951a24fccecd958416856cecef444 uncommitted
--- a/sys/src/cmd/skelfs.c
+++ b/sys/src/cmd/skelfs.c
@@ -101,9 +101,6 @@
 
 	s = old->aux;
 	s2 = emalloc9p(sizeof *s2);
-	if(s2 == nil)
-		return "out of memory";
-	memset(s2, 0, sizeof *s2);
 
 	s2->mode = s->mode;
 	utfecpy(s2->name, &s2->name[sizeof s2->name-1], s->name);

Re: [9front] [patch] skelfs: remove useless code

[Jacob Moody]

On 11/11/22 10:17, Arne Meyer wrote:
> Hello,
>   
> if I read the code correctly emalloc9p already checks for failed allocations and zeroes the memory, so the following code is redundant.
> 
> Greetings,
> Arne

You are correct, muscle memory did not work in my favor this time.

Thanks!
moody

Re: [9front] [patch] skelfs: remove useless code

[Arne Meyer]

After that patch I grepped through /sys/src/cmd/* and found a more hits. Is there any difference between handrolled emalloc implementations and emalloc9p? If not, I can go spelunking to replace emalloc where emalloc9p is available.

> Jacob Moody <moody@mail.posixcafe.org> hat am 11.11.2022 17:37 GMT geschrieben:
> 
>  
> On 11/11/22 10:17, Arne Meyer wrote:
> > Hello,
> >   
> > if I read the code correctly emalloc9p already checks for failed allocations and zeroes the memory, so the following code is redundant.
> > 
> > Greetings,
> > Arne
> 
> You are correct, muscle memory did not work in my favor this time.
> 
> Thanks!
> moody

Re: [9front] [patch] skelfs: remove useless code

[Jacob Moody]

On 11/11/22 10:52, Arne Meyer wrote:
> After that patch I grepped through /sys/src/cmd/* and found a more hits. Is there any difference between handrolled emalloc implementations and emalloc9p? If not, I can go spelunking to replace emalloc where emalloc9p is available.
> 
>> Jacob Moody <moody@mail.posixcafe.org> hat am 11.11.2022 17:37 GMT geschrieben:
>>
>>  
>> On 11/11/22 10:17, Arne Meyer wrote:
>>> Hello,
>>>   
>>> if I read the code correctly emalloc9p already checks for failed allocations and zeroes the memory, so the following code is redundant.
>>>
>>> Greetings,
>>> Arne
>>
>> You are correct, muscle memory did not work in my favor this time.
>>
>> Thanks!
>> moody

The only difference I generally see is that the emalloc9p and friends will call exits("mem"), which in the context
of lib9p (generally) just means the process for that specific pipe is killed. I believe the intention here is to allow other srv
procs to continue existing, but if that is the case we surely are doing it half assed right now.

For starters we should probably be using threadexits() when we're threaded, and we're surely leaking entries in the
fidmaps and the Fid structs themselves. Ideally those functions would be given enough context to properly unroll
the entire process state, if we did want to really handle this case. Of course this is only for file systems
that do have that listen/fork loop, if you are just posting to /srv then you will only ever have the one service
proc. But then of course for threaded systems you have to communicate that things blew up, or else your srv
proc could just die and leave everything else deadlocked.

So here's my shot in the dark for something that could be a bit better, if we had the e* functions in mem.c
take a Request as the first argument, then perhaps provide a destroysrv callback for user code to clean things up
associated with that pipe. With that those functions have a better idea of how to clean up the mess. But these error
handling unrolling things are almost all some brand of awful, it may not be worth the effort to do 'better'.

While reviewing the lib9p code for these thoughts, I came across a scenario of lib9p checking for this
stuff itself, this changes that:

diff a39eb30ffa2138a817bd787fdfd25eb65927e7d4 uncommitted
--- a//sys/src/lib9p/srv.c
+++ b//sys/src/lib9p/srv.c
@@ -666,10 +666,6 @@
 	}
 	n = GBIT16(tmp)+BIT16SZ;
 	statbuf = emalloc9p(n);
-	if(statbuf == nil){
-		r->error = "out of memory";
-		return;
-	}
 	r->ofcall.nstat = convD2M(&r->d, statbuf, n);
 	r->ofcall.stat = statbuf;	/* freed in closereq */
 	if(r->ofcall.nstat <= BIT16SZ){

Re: [9front] [patch] skelfs: remove useless code

[Arne Meyer]

If we are failing to allocate, why try to keep things running? We could just sysfatal. Most emalloc functions do exactly that, and some code uses both emalloc and emalloc9p.

> Jacob Moody <moody@mail.posixcafe.org> hat am 11.11.2022 19:20 GMT geschrieben:
> 
>  
> On 11/11/22 10:52, Arne Meyer wrote:
> > After that patch I grepped through /sys/src/cmd/* and found a more hits. Is there any difference between handrolled emalloc implementations and emalloc9p? If not, I can go spelunking to replace emalloc where emalloc9p is available.
> > 
> >> Jacob Moody <moody@mail.posixcafe.org> hat am 11.11.2022 17:37 GMT geschrieben:
> >>
> >>  
> >> On 11/11/22 10:17, Arne Meyer wrote:
> >>> Hello,
> >>>   
> >>> if I read the code correctly emalloc9p already checks for failed allocations and zeroes the memory, so the following code is redundant.
> >>>
> >>> Greetings,
> >>> Arne
> >>
> >> You are correct, muscle memory did not work in my favor this time.
> >>
> >> Thanks!
> >> moody
> 
> The only difference I generally see is that the emalloc9p and friends will call exits("mem"), which in the context
> of lib9p (generally) just means the process for that specific pipe is killed. I believe the intention here is to allow other srv
> procs to continue existing, but if that is the case we surely are doing it half assed right now.
> 
> For starters we should probably be using threadexits() when we're threaded, and we're surely leaking entries in the
> fidmaps and the Fid structs themselves. Ideally those functions would be given enough context to properly unroll
> the entire process state, if we did want to really handle this case. Of course this is only for file systems
> that do have that listen/fork loop, if you are just posting to /srv then you will only ever have the one service
> proc. But then of course for threaded systems you have to communicate that things blew up, or else your srv
> proc could just die and leave everything else deadlocked.
> 
> So here's my shot in the dark for something that could be a bit better, if we had the e* functions in mem.c
> take a Request as the first argument, then perhaps provide a destroysrv callback for user code to clean things up
> associated with that pipe. With that those functions have a better idea of how to clean up the mess. But these error
> handling unrolling things are almost all some brand of awful, it may not be worth the effort to do 'better'.
> 
> While reviewing the lib9p code for these thoughts, I came across a scenario of lib9p checking for this
> stuff itself, this changes that:
> 
> diff a39eb30ffa2138a817bd787fdfd25eb65927e7d4 uncommitted
> --- a//sys/src/lib9p/srv.c
> +++ b//sys/src/lib9p/srv.c
> @@ -666,10 +666,6 @@
>  	}
>  	n = GBIT16(tmp)+BIT16SZ;
>  	statbuf = emalloc9p(n);
> -	if(statbuf == nil){
> -		r->error = "out of memory";
> -		return;
> -	}
>  	r->ofcall.nstat = convD2M(&r->d, statbuf, n);
>  	r->ofcall.stat = statbuf;	/* freed in closereq */
>  	if(r->ofcall.nstat <= BIT16SZ){