[pulseaudio-discuss] [PATCH] core, modules: Remove useless EINTR tests

[Pali Rohár]

On Thursday 16 January 2020 17:31:17 Tanu Kaskinen wrote:
> Resurrecting this old patch...
> 
> On Mon, 2019-06-03 at 15:43 +0200, Pali Rohár wrote:
> > On Tuesday 28 May 2019 16:49:19 Frédéric Danis wrote:
> > > Since commit ad447d14682 (in 2009) pa_read and pa_write take care of
> > > handling EINTR error.
> > > So, pa_read, pa_write, pa_iochannel_read and pa_iochannel_write can not
> > > exit with errno set to EINTR, and testing it is useless.
> > > ---
> > >  src/modules/bluetooth/module-bluez5-device.c | 16 +-------
> > >  src/modules/module-esound-sink.c             |  4 +-
> > >  src/modules/module-pipe-sink.c               | 17 ++++-----
> > >  src/modules/module-pipe-source.c             |  4 +-
> > >  src/modules/module-solaris.c                 |  4 +-
> > >  src/modules/oss/module-oss.c                 | 10 +----
> > >  src/pulsecore/fdsem.c                        | 40 ++++++--------------
> > >  src/pulsecore/iochannel.c                    |  2 +-
> > >  src/pulsecore/protocol-esound.c              |  8 ++--
> > >  src/pulsecore/protocol-simple.c              |  2 +-
> > >  10 files changed, 32 insertions(+), 75 deletions(-)
> > > 
> > > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> > > index 56c96054d..f850a3a41 100644
> > > --- a/src/modules/bluetooth/module-bluez5-device.c
> > > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > > @@ -279,10 +279,6 @@ static int sco_process_render(struct userdata *u) {
> > >  
> > >          saved_errno = errno;
> > >  
> > > -        if (saved_errno == EINTR)
> > > -            /* Retry right away if we got interrupted */
> > > -            continue;
> > > -
> > >          pa_memblock_unref(memchunk.memblock);
> > >  
> > >          if (saved_errno == EAGAIN) {
> > > @@ -445,11 +441,7 @@ static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
> > >  
> > >          if (l < 0) {
> > >  
> > > -            if (errno == EINTR)
> > > -                /* Retry right away if we got interrupted */
> > > -                continue;
> > > -
> > > -            else if (errno == EAGAIN) {
> > > +            if (errno == EAGAIN) {
> > >                  /* Hmm, apparently the socket was not writable, give up for now */
> > >                  pa_log_debug("Got EAGAIN on write() after POLLOUT, probably there is a temporary connection loss.");
> > >                  break;
> > > @@ -543,11 +535,7 @@ static int a2dp_process_push(struct userdata *u) {
> > >  
> > >          if (l <= 0) {
> > >  
> > > -            if (l < 0 && errno == EINTR)
> > > -                /* Retry right away if we got interrupted */
> > > -                continue;
> > > -
> > > -            else if (l < 0 && errno == EAGAIN)
> > > +            if (l < 0 && errno == EAGAIN)
> > >                  /* Hmm, apparently the socket was not readable, give up for now. */
> > >                  break;
> > >  
> > 
> > Hi! This change conflicts with "Implement reading SO_TIMESTAMP for A2DP
> > source" patch as it stops using pa_read() function. For SO_TIMESTAMP is
> > needed recvmsg() and then also handling of EINTR/EAGAIN.
> 
> Thanks for pointing this out. It would have been easy for me to miss
> this, since git rebased the patch without conflicts.

This is because pa_read() was replaced by recvmsg() in bluetooth code.
So above information about EINTR is not valid for bluetooth code which
reads data, and needs to properly handle EINTR error code.

> fdsem.c had changes like this:
> 
> > @@ -151,11 +151,8 @@ static void flush(pa_fdsem *f) {
> >              uint64_t u;
> >  
> >              if ((r = pa_read(f->efd, &u, sizeof(u), NULL)) != sizeof(u)) {
> > -
> > -                if (r >= 0 || errno != EINTR) {
> > -                    pa_log_error("Invalid read from eventfd: %s", r < 0 ? pa_cstrerror(errno) : "EOF");
> > -                    pa_assert_not_reached();
> > -                }
> > +                pa_log_error("Invalid read from eventfd: %s", r < 0 ? pa_cstrerror(errno) : "EOF");
> > +                pa_assert_not_reached();
> >  
> >                  continue;
> 
> The "continue" statement after the assertion is unnecessary, so I
> removed it.
> 
> I created an MR for this patch, since we now push all changes through
> GitLab: 
> https://gitlab.freedesktop.org/pulseaudio/pulseaudio/merge_requests/238
> 
> I didn't merge the change yet, because we're currently in freeze. I'll
> merge it once 14.0 is out.
> 
> Thanks for the patch, Frédéric, and sorry for the long delay!
> 

-- 
Pali Rohár
pali.rohar at gmail.com