>>[…]
>It's also used in fports.c in the patch -- we need the helper so we can
get the errno in order to handle ENODEV in fport_print.
Ok, I think I assumed everything was being done in one file and glossed over
the file names.
>> Also, the locking is like...
>OK, so you're saying that the _existing_ scm_ttyname() is potentially
broken in these ways. Assuming so, then I might see about handling that
right now, but I think it could also be filed as a bug for later.
(emphasis added) To be clear, it’s the new version I looked at (unless I
misread some + for - or something). But since the new is based on the old …
>And depending on what you mean by "non-standard", I don't know whether
that change (e.g. to allow EINTR to propagate, and to stop processing
ticks) could go into a stable release.
“non-standard”, as in:
• EINTR is not in the list of errors mentioned by POSIX (so would never happen
on correct POSIX systems)
• There does not appear to be a compelling reason to deviate from POSIX.
• Even without POSIX stating it, it seems hard to find a good reason to return
EINTR.
• Did you ever see (outside somewhat overzealous automation) any code that
checks for EINTR? If you are implementing a kernel, better don’t return EINTR,
or it would break things.
• I don’t know _any_ system that has ttyname return EINTR (Hurd theoretically
can, but that’s for custom POSIX-violating term servers making rather … dubious
choices).
(You could still retry after EINTR, but you would need to implement a loop
yourself: something like do { lock(); syscall(); save errno; unlock(); tick}
while(error == EINTR) }.)
On ticks: it’s not that ticks stop being processed, rather (assuming you move
the async tick outside the locking/unlocking but still keep it in the function)
that they won’t be processed right next to the syscall.
Also, while I wouldn’t recommend it, it probably wouldn’t cause issues in the
wild to remove the async tick entirely for scm_ttyname (or scm_c_i_ttyname,
whatever) - unless there is code in the wild that (1) is written in C (the
Scheme equivalent isn’t affected because the Scheme compiler automatically
inserts async ticks (*) in case of loop constructs) and (2) does something like:
// lots of iterations, doesn’t have to be actually infinite
while(1) {
scm_ttyname(something)
// and don’t call any I/O function here,
// and don’t call any other Guile syscall-like function that likely do an
(possibly indirect) async tick
// (also some other Guile functions may have safe points)
// and don’t call any interesting Scheme procedures (if (*) is true, don’t
call any Scheme procedure at all)
// and don’t a manual SCM_ASYNC_TICK (or was it SCM_TICK?)
}
// ^ why would you even do this???
(*) I think it also does this for procedure calls but I’m not sure.
(To be clear, I’m only saying this about ttyname. There likely are many other
syscalls where you can shuffle the safepoint/tick around a little, but I’m not
making a general claim here.)
>In any case, I'm happy to fix all the problems if we can be confident
we're not causing a regression, and not introducing something
inappropriate for a Z release -- I'll probably look into it a bit
further today, but for now, my more immediate goal is to fix the test by
fixing the ENODEV problem, without making anything worse.
I, for one, am confident that the proposed async tick adjustments for ttyname
does not cause a regression.
I’m not really confident about what’s all going on with buffer sizes and
maximum length, and even if the interpretation of POSIX used here is _correct_,
it might be the case that the OS interpretation is wrong and we need to deal
with it.
(Perhaps looking at the Linux kernel side may be a good check?)
> On length checks and the like: it isn’t clear to me whether
> TTY_NAME_MAX includes the terminating zero or not (I would expect not,
> like strlen).
>I wsn't sure at first either, but then I saw that POSIX says "The value
of namesize is smaller than the length of the string to be returned
including the terminating null character" for the ERANGE case, so I
assumed it was included:
>https://pubs.opengroup.org/onlinepubs/9699919799/functions/ttyname.html
True, but that’s ‘namesize’, not a hypothetical ‘namelength’. And the
description in the beginning says “the maximum _length_ of the terminal name
shall be {TTY_NAME_MAX}”. But is that the specified behaviour or is it sloppy
writing?
If only POSIX said either ‘(excluding terminating zero)’ or ‘(including
terminating null character)’, that would have made things clear, but no …
All that said, perhaps just use ‘TTY_NAME_MAX+1’ instead of ‘TTY_NAME_MAX’?
It’s not like we care here about not exceeding the nominal maximum for its own
sake, rather it’s just to have a sufficiently large buffer.
Best regards,
Maxime Devos
