[PATCH] do not check for POLLERR, POLLHUP, POLLNVAL when reading data

[Pavel Březina]

Read the commit message for more info.

This was causing troubles when I wanted the pam responder to close 
connection to a client immediately, right after a request is finished.

This code has been in SSSD for years. Simo, as you are the original 
author, can you review it please?

[Simo Sorce]

On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
>  Read the commit message for more info.
>  
>  This was causing troubles when I wanted the pam responder to close 
>  connection to a client immediately, right after a request is finished.
>  
>  This code has been in SSSD for years. Simo, as you are the original 
>  author, can you review it please? 
Uhmm following your explanation it looks like just removing the check
would be wrong.
Shouldn't you instead catch POLLHUP indipendently and simply add a flag
somewhere that the pipe should be close after you successfully completed
the read ?

Or is there something else I am missing here ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Simo Sorce]

On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
>  From 7c49fe46a3fbce1433ad1680a2e12335542706ed Mon Sep 17 00:00:00
> 2001
>  From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  Date: Thu, 4 Oct 2012 11:42:59 +0200
>  Subject: [PATCH 1/3] do not check for POLLERR, POLLHUP, POLLNVAL when
>  reading
>   data
>  
>  According to the poll manpage, those error are valid only for
>  output only.
>   
Ah btw I wanted to ask what you mean with this comment in the commit
message.

When 'reading' the flags after the poll returns we are in the 'output'
situation, but from the way you wrote the comment to me it reads like
you mention 'output' as meaning 'write event' (ie POLLOUT) ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Pavel Březina]

On 10/04/2012 04:22 PM, Simo Sorce wrote:
>  On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
> >  From 7c49fe46a3fbce1433ad1680a2e12335542706ed Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
> > Date: Thu, 4 Oct 2012 11:42:59 +0200
> > Subject: [PATCH 1/3] do not check for POLLERR, POLLHUP, POLLNVAL when
> > reading
> >   data
> >
> > According to the poll manpage, those error are valid only for
> > output only.
> >
>  Ah btw I wanted to ask what you mean with this comment in the commit
>  message.
>
>  When 'reading' the flags after the poll returns we are in the 'output'
>  situation, but from the way you wrote the comment to me it reads like
>  you mention 'output' as meaning 'write event' (ie POLLOUT) ?
>
>  Simo.
>  
Oh, ignore anything I wrote before please. I completely misunderstood 
the documentation.

I'm sending new patch. It special cases POLLHUP. If it occurs, we will 
read the pipe and then close fd on the client side.

[Simo Sorce]

On Fri, 2012-10-05 at 10:48 +0200, Pavel Březina wrote:
>  On 10/04/2012 04:22 PM, Simo Sorce wrote:
>  > On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
>  >>  From 7c49fe46a3fbce1433ad1680a2e12335542706ed Mon Sep 17 00:00:00 2001
>  >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  >> Date: Thu, 4 Oct 2012 11:42:59 +0200
>  >> Subject: [PATCH 1/3] do not check for POLLERR, POLLHUP, POLLNVAL when
>  >> reading
>  >>   data
>  >>
>  >> According to the poll manpage, those error are valid only for
>  >> output only.
>  >>
>  > Ah btw I wanted to ask what you mean with this comment in the commit
>  > message.
>  >
>  > When 'reading' the flags after the poll returns we are in the
> 'output'
>  > situation, but from the way you wrote the comment to me it reads like
>  > you mention 'output' as meaning 'write event' (ie POLLOUT) ?
>  >
>  > Simo.
>  >
>  
>  Oh, ignore anything I wrote before please. I completely misunderstood 
>  the documentation.
>  
>  I'm sending new patch. It special cases POLLHUP. If it occurs, we will 
>  read the pipe and then close fd on the client side. 
Looks good to me now.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

On Mon, Oct 08, 2012 at 10:55:20AM -0400, Simo Sorce wrote:
>  On Fri, 2012-10-05 at 10:48 +0200, Pavel Březina wrote:
>  > On 10/04/2012 04:22 PM, Simo Sorce wrote:
>  > > On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
>  > >>  From 7c49fe46a3fbce1433ad1680a2e12335542706ed Mon Sep 17 00:00:00 2001
>  > >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > >> Date: Thu, 4 Oct 2012 11:42:59 +0200
>  > >> Subject: [PATCH 1/3] do not check for POLLERR, POLLHUP, POLLNVAL when
>  > >> reading
>  > >>   data
>  > >>
>  > >> According to the poll manpage, those error are valid only for
>  > >> output only.
>  > >>
>  > > Ah btw I wanted to ask what you mean with this comment in the commit
>  > > message.
>  > >
>  > > When 'reading' the flags after the poll returns we are in the
> 'output'
>  > > situation, but from the way you wrote the comment to me it reads like
>  > > you mention 'output' as meaning 'write event' (ie POLLOUT) ?
>  > >
>  > > Simo.
>  > >
>  > 
>  > Oh, ignore anything I wrote before please. I completely misunderstood 
>  > the documentation.
>  > 
>  > I'm sending new patch. It special cases POLLHUP. If it occurs, we will 
>  > read the pipe and then close fd on the client side.
>  
>  Looks good to me now.
>  
>  Simo. 
Looks good to me, too.

Ack.

[Jakub Hrozek]

On Wed, Oct 10, 2012 at 03:26:03PM +0200, Jakub Hrozek wrote:
>  On Mon, Oct 08, 2012 at 10:55:20AM -0400, Simo Sorce wrote:
>  > On Fri, 2012-10-05 at 10:48 +0200, Pavel Březina wrote:
>  > > On 10/04/2012 04:22 PM, Simo Sorce wrote:
>  > > > On Thu, 2012-10-04 at 13:03 +0200, Pavel Březina wrote:
>  > > >>  From 7c49fe46a3fbce1433ad1680a2e12335542706ed Mon Sep 17 00:00:00
> 2001
>  > > >> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com&gt;
>  > > >> Date: Thu, 4 Oct 2012 11:42:59 +0200
>  > > >> Subject: [PATCH 1/3] do not check for POLLERR, POLLHUP, POLLNVAL when
>  > > >> reading
>  > > >>   data
>  > > >>
>  > > >> According to the poll manpage, those error are valid only for
>  > > >> output only.
>  > > >>
>  > > > Ah btw I wanted to ask what you mean with this comment in the commit
>  > > > message.
>  > > >
>  > > > When 'reading' the flags after the poll returns we are in the
> 'output'
>  > > > situation, but from the way you wrote the comment to me it reads like
>  > > > you mention 'output' as meaning 'write event' (ie POLLOUT)
> ?
>  > > >
>  > > > Simo.
>  > > >
>  > > 
>  > > Oh, ignore anything I wrote before please. I completely misunderstood 
>  > > the documentation.
>  > > 
>  > > I'm sending new patch. It special cases POLLHUP. If it occurs, we will 
>  > > read the pipe and then close fd on the client side.
>  > 
>  > Looks good to me now.
>  > 
>  > Simo.
>  
>  Looks good to me, too.
>  
>  Ack. 
Pushed to master.