9front - general discussion about 9front
 help / color / mirror / Atom feed
From: Jacob Moody <moody@mail.posixcafe.org>
To: 9front@9front.org
Subject: Re: [9front] [patch] skelfs: remove useless code
Date: Fri, 11 Nov 2022 12:20:25 -0700	[thread overview]
Message-ID: <94a397b1-daa1-2ad2-137b-64dbbd150065@posixcafe.org> (raw)
In-Reply-To: <1453278421.4596864.1668189134709@comcenter.netcologne.de>

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-11 19:24 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 [this message]
2022-11-12 10:20       ` Arne Meyer

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:

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

  git send-email \
    --in-reply-to=94a397b1-daa1-2ad2-137b-64dbbd150065@posixcafe.org \
    --to=moody@mail.posixcafe.org \
    --cc=9front@9front.org \


* 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).