Hi Olivier !
Thanks for the patches. A bit of discussion below.
> So I wanted to have the return code/signal that occured when a process went down
> available on the statusfile, as I believe this is useful information to have for
> all services.
Yes, that makes sense.
> Looking into it, I noticed that s6 already did gather that info from wait(), and
> sends it to ./finish -- that was a nice surprise, but (unless I missed it?)
> totally undocumented.
I was certain I had documented it somewhere, but I must have been wrong, because
I couldn't find it. So, thanks for the report, and I'll apply that patch or
update the doc somehow.
> First patch fixes the first argument to ./finish, since I assume it was meant to
> be able to know whether there was a return code or not, but 255 actually is a
> valid return code. Second patch adds a mention of those arguments to the doc.
Yes, I didn't think too much about that, but in hindsight, since it's possible
to use a code outside of the valid range, it's the right thing to do indeed.
What bothers me, though, is that it breaks existing scripts, so it requires a
major (as in second number in my 4-number versioning system) number change,
loud horns and blinking lights. It can't be the sneaky patch. Totally my
fault, but compatibility breaks are never fun.
> Then third patch does add that info to the statusfile
And here I'm slowing down. I agree it makes sense for the info to be there.
However, wstat is specified as an int, and the mapping of the bits inside
a wstat is totally unspecified, even if most implementations are the same
in practice. That means that wstat cannot be stored as an uint32 in the
status file; if anything, it should be stored as an uint64, to accommodate
archs where int is 64 bits. Adding 8 bytes to the status file is heavy
(all things are relative...) I'll probably end up doing that, but I need to
think about it a bit more first.
> and finally as a separate
> patch I've also added the signal name to s6-svstat as I think it's a nice thing
> to have.
I'm reluctant to add a number-to-signal-name conversion to a single program.
It really should be a libc function. If anything, I'll add it to skalibs, so
other programs can use the conversion too.
More importantly, the s6-svstat output is meant to be parsable, and changing
the numbers into signal names makes parsing harder. So there should be at
least an option to deactivate the conversion.
Next time, please express wishes before sending patches - I like design
discussions and agreement on what to do, I kinda feel trapped or forced when
I get patches, as in "I worked hard on this so you better apply them!".
I have to say your patches are pretty much on-point, though, and almost
applicable as is, so thank you for that.
--
Laurent
Received on Sun Jan 18 2015 - 23:34:15 UTC