Re: CVS commit: src/sys/kern

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Fri, 30 Sep 2022 20:15:07 -0400
    From:        Christos Zoulas <christos%zoulas.com@localhost>
    Message-ID:  <AB0EA4EE-C3B9-4237-BCA3-C1672EB4FD8A%zoulas.com@localhost>

  | It does not need an extra flag (it looks in the file descriptor flags to
  | find if it needs to set or not.

One of us is confused.   From where in this case does anything
get the exclose flag set?   That's the whole question here.  The
flags arg that is passed around has O_CLOEXEC set in it - you used
that in the call to fd_set_exclose() in kern/tty_ptm.c ... but where
you said that would be better done in fd_affix().

That does not have access to the flags.   So from where is it going
to get the close on exec info ?

My reading of do_open() is that the O_CLOEXEC flag is never even
examined when a cloning device is opened, it doesn't get set on
the original fd (the cloner) or the cloned device (other than by
your recent modification for /dev/pmx).

Did I misread the code?

Or are you planning something different than it seemed?

  | to find other cases where we forgot to call fd_set_exclose() before calling
  | fd_affix().

My point is that it should not be necessary to call fd_set_exclose()
in every (or any) cloning device driver.  The open syscall handling
is where that should be done, just as it is for all the opens that
are not cloning devices.

Why be different?

  | It also does not need locking because the process can't access
  | the descriptor before calling fd_affix.

The locking I was referring to are the vnode locks/references in
do_open(), not anything related to the file struct or descriptor.
I just do not feel competent to get all of that correct in this
case (more complex than the normal case because of the extra vnode
involved) and would prefer if someone familiar with all of that
were to handle it - particularly in the extra error case that will
need to be handled, even if I cannot see how it would actually fire
in the case in question.

kre