Re: Q: tty_open() && hangup

[Oleg Nesterov]

Hi Peter,

On 04/15, Peter Hurley wrote:
> >
> > If we just want to add a preemption point then perhaps we should use
> > cond_resched/might_resched ?
>
> Yeah, it's just a preemption point that pre-dates cond_resched.

OK, so perhaps s/schedule/might_reched/ makes sense to make it more
clear.
> > So we can only get here if tty->ops->open returns -ERESTARTSYS without
> > signal_pending(). This doesn't look good. But OK, lets forget this.
>
> tty_port_block_til_ready() returns -ERESTARTSYS if !ASYNC_HUP_NOTIFY and
> tty was hungup or the h/w was reset

Yes, yes, I saw this code, and I am not saying this is buggy.

I meant this looks confusing. At least to me, until I grepped for ERESTARTSYS
I thought that these code paths do something non-trivial with signal handling.

IMHO, ERESTARTSYS should only be used if signal_pending() is true and we are
going to return this error code to user-space. But tty_port_block_til_ready()
returns ERESTARTSYS if !ASYNC_HUP_NOTIFY _or_ signal_pending(), that is why
tty_open() has to check signal_pending() too.

I think this deserves a cleanup, but this is minor and of course subjective,
please forget.

> >   2148                  /*
> >   2149                   * Need to reset f_op in case a hangup happened.
> >   2150                   */
> >   2151                  if (tty_hung_up_p(filp))
> >   2152                          filp->f_op = &tty_fops;
> >
> > And this is called after tty_add_file() which makes this filp visible to
> > __tty_hangup(), and without tty_lock().
>
> tty_release() has removed the filp from the files list already (with the
> tty lock held),

Heh. Can't understand how did I miss that ;)

Thanks Peter.

Oleg.