[Dropped onf@ per their request; updated Ken's address] Hi Jacob,
On Sun, Feb 09, 2025 at 06:21:42PM -0600, Jacob Moody wrote:
> >> In general 9front specific comments should go to the 9front list
> >> 9fr...@9front.org
> >> more general questions regarding any sort of plan 9 may be best sent to
> >> 9fans@9fans.net
> >> There is enough overlap that it should be seen by the right people.
> >
> > Thanks! Are those lists public? That is, can I post to those lists
> > without being subscribed?
>
> 9fans is, 9front is not.
Ok.
> > Iff we can't fix seprint(2) for historic reasons, I think it would be
> > better to invent a new seprint2(2) and consider slowly migrating all
> > code to that API. Existing code using seprint(2) might have many bugs,
> > and needs to be audited anyway.
>
> Sure let's audit existing code and fix things, I am very much interested
> in making things better, but I don't think we should point at a couple of bugs
> and decide we need to rip up the floor boards. More of the reasoning for why
> I feel this way is later on in this mail.
[...]
> > 200- for(;;){
> > 201- nc = nr = 0;
> > 202- while(cp[nc] != 0){
> > 203- n = chartorune(&r, cp+nc);
> > 204- if(r == '.')
> > 205- break;
> > 206- if(nr >= nelem(rb))
> > 207- return -1;
> > 208- rb[nr++] = r;
> > 209- nc += n;
> > 210- }
> > 211- if(cistrncmp(cp, "xn--", 4) == 0)
> > 212- if((nr = punydecode(nc-4, cp+4, nelem(rb), rb))
> > < 0)
> > 213- return -1;
> > 214: if((dp = seprint(dp, de, "%.*S", nr, rb)) == nil)
> > 215- return -1;
> > 216- if(dp >= de)
> > 217- return -1;
> > 218- if(cp[nc] == 0)
> > 219- break;
> > 220- *dp++ = '.';
> > 221- cp += nc+1;
> > 222- }
> >
> > In line 214, seprint(2) cannot possibly return anything >=de. And
> > there's no increment between line 214 and line 216, so line 216 must be
> > dead code, isn't it? The value of dp might be incremented later in line
> > 220, but that's already after the test has happened, and thus cannot
> > make it non-dead code posthumously, can it?
>
> Yes line 216 is dead code, which is why it was removed in the new code.
> Your original comment was that the check on line 214 was impossible,
> I was commenting that no that check on line 214 is intentional.
> Sorry, I should have been more specific.
Hmmm, makes sense.
> >> and relying on this behavior is not great
> >> so we are exploring perhaps changing this to make things more clear.
> >> http://okturing.com/src/23249/body is the current draft.
> >
> > This looks better, but I'm not happy doing a transformation like that
> > claiming that nothing changes, where the current code clearly reads as
> > dead code. I'd first make sure that the commit message really tells the
> > full story.
>
> We just pushed a change that we're happy with.
> Thanks for the bug report.
You're welcome! I've fetched the changes, and after your clarification,
they look good to me (I would have appreciated a mention about the
report(s) in the commit message, though:). Thanks for fixing them!
> >> In my personal opinion the issue is not so great that it would be worth
> >> changing
> >> the existing code (and all the callers). More "core" functions like this
> >> is used not
> >> only by internal code but by plenty of external code.
> >
> > I suspect most of that external code is bogus, and probably depends on
> > some behavior that differs from the actual behavior of seprint(2), just
> > like the code we discussed above has dead code attempting to check for
> > truncation with invalid tests.
> >
> > Here's one that's more obviously wrong, I think:
> >
> > sys/src/cmd/nusb/lib/dump.c:75: s = seprint(s, e, " %.2ux",
> > b[i]);
> > sys/src/cmd/nusb/lib/dump.c-76- if(s == e)
> > sys/src/cmd/nusb/lib/dump.c-77- fprint(2, "%s: usb/lib:
> > hexdump: bug: small buffer\n", argv0);
> > sys/src/cmd/nusb/lib/dump.c-78- return dbuff;
> > sys/src/cmd/nusb/lib/dump.c-79-}
> >
> > s==e cannot be possibly true in line 76 right after the call to
> > seprint(2) in line 75. That's necessarily dead code, or I'm getting
> > lost in the implementation of vseprint(2).
>
> You're correct in your interpretation, this bug is also fixed now.
> Thanks for the reports, if you find any more I'd be very happy to
> have them sent our way.
Nice! :)
> > After checking many call sites, I think I agree with you that the API
> > cannot be fixed without breaking some code (code that is already broken
> > in other ways, but). Maybe I'd add a new API and slowly move to it,
> > deprecating seprint(2). Maybe call the new one seprint2(2)?
>
> Maybe, we had talked about doing something like this after your first email.
> There was a lack of consensus on if we'd like to do this at the moment.
Feel free to CC me on those emails. I enjoy discussing these things . :)
> >> These days I think we like to
> >> have a fairly good reason to grow the list of 9front idiosyncrasies
> >> compared to the
> >> original plan 9 for core interfaces like this.
> >
> > I'd say having this bad API being code is in itself a problem, and a
> > better API would probably fix some bugs, and promote better code.
>
> It's a little funky and perhaps a little sharp, but that is not something
> uncommon in C.
Yup.
> I can agree with you that simpler semantics would have been nice, but I think
> you're missing
> some context of how the plan 9 community works at this point. We're really
> quite small and the
> churn rate has historically been really low (9front has picked up some steam
> in comparison but
> is still not anywhere near most other mainstream operating systems). We have
> people that use
> quite old code all the time, or have their own existing code that might be
> going on a couple
> decades old at this point. This is in part because "core" APIs don't tend to
> change or churn that
> much. We have community members who are active that still do a lot of work on
> "original"
> plan 9 with minimal patches. Things have drifted throughout the years, some
> drift more warranted
> then others. We still like to share code around and I am bit hesitant to make
> things harder
> to move back and forth.
Heh, a little bit ironic that the OS that departed from Unix to not
depend on 15-year-old mistakes and be free to innovate does now depend
on 40-year-old mistakes. :D
I understand, though.
> Anyway, all this is to just give some context about some of our hesitation to
> make some changes
> for an issue on paper that is annoying but is still possible to avoid.
>
> I appreciate your review on the 9front code, always happy to have discussions
> like this :).
We'll probably have more. :)
The thing is, I've been researching on the C string APIs (<string.h>
and <strings.h>) for a few years already, auditing the shadow-utils
source code in Linux, and developing newer APIs that allow us to write
better code, free of bugs, while keeping the spirit of the string APIs.
That's how I ended up writing my own seprint(2)-like API. (I guess Rob
and Ken had similar itches back then when they wrote it.) That API has
already made me find and fix several bugs in shadow-utils.
I'm preparing to publish a string library, libs, with all these APIs,
and was checking existing code that looks similar, in case anything can
be done better. Since I would like to take over the name (or actually,
a similar one), by naming my API seprintf() (it currently is named
stpeprintf()), it would be interesting if both had as-close-as-possible
semantics, to avoid bugs when switching from one to the other.
I will probably continue reading your source code every now and then,
and might report similar bugs in string-handling code. And especially,
design bugs in APIs. ;)
(BTW, strecpy(2) has similar design issues.)
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
signature.asc
Description: PGP signature
------------------------------------------ 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/T90be9e76a4dae77d-Ma6284b475fc32805cb9547fa Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
