9front - general discussion about 9front
 help / color / mirror / Atom feed
From: Arne Meyer <meyer.arne83@netcologne.de>
To: 9front@9front.org
Subject: Re: [9front] [patch] skelfs: remove useless code
Date: Sat, 12 Nov 2022 11:20:12 +0100 (CET)	[thread overview]
Message-ID: <305235037.4605417.1668248412799@comcenter.netcologne.de> (raw)
In-Reply-To: <94a397b1-daa1-2ad2-137b-64dbbd150065@posixcafe.org>

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){

      reply	other threads:[~2022-11-12 10:21 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-11 17:17 Arne Meyer
2022-11-11 17:37 ` Jacob Moody
2022-11-11 17:52   ` Arne Meyer
2022-11-11 19:20     ` Jacob Moody
2022-11-12 10:20       ` Arne Meyer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=305235037.4605417.1668248412799@comcenter.netcologne.de \
    --to=meyer.arne83@netcologne.de \
    --cc=9front@9front.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).