Re: [pulseaudio-discuss] [PATCH] core, modules: Remove useless EINTR tests
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, , 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.ro...@gmail.com
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] core, modules: Remove useless EINTR tests
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.
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, , 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!
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] core, modules: Remove useless EINTR tests
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.
--
Pali Rohár
pali.ro...@gmail.com
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] core, modules: Remove useless EINTR tests
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;
diff --git a/src/modules/module-esound-sink.c b/src/modules/module-esound-sink.c
index 5ff04516a..f46dc3889 100644
--- a/src/modules/module-esound-sink.c
+++ b/src/modules/module-esound-sink.c
@@ -249,9 +249,7 @@ static void thread_func(void *userdata) {
if (l < 0) {
-if (errno == EINTR)
-continue;
-else if (errno == EAGAIN) {
+if (errno == EAGAIN) {
/* OK, we filled all socket buffers up
* now. */
diff --git a/src/modules/module-pipe-sink.c b/src/modules/module-pipe-sink.c
index 213924fdc..43595420f 100644
--- a/src/modules/module-pipe-sink.c
+++ b/src/modules/module-pipe-sink.c
@@ -199,14 +199,13 @@ static ssize_t pipe_sink_write(struct userdata *u,
pa_memchunk *pchunk) {
if (l < 0) {
if (errno == EAGAIN)
break;
-else if (errno != EINTR) {
-if (!u->fifo_error) {
-pa_log("Failed to write data to FIFO: %s",
pa_cstrerror(errno));
-u->fifo_error = true;
-}
-count = -1 - count;
-break;
+
+if (!u->fifo_error) {
+pa_log("Failed to write data to FIFO: %s",
pa_cstrerror(errno));
+u->fifo_error = true;
}
+count = -1 - count;
+break;
} else {
if (u->fifo_error) {
pa_log_debug("Recovered from FIFO error");
@@ -288,9 +287,7 @@ static int process_render(struct userdata *u) {
if (l < 0) {
-if (errno == EINTR)
-continue;
-else if (errno == EAGAIN)
+if (errno == EAGAIN)
return 0;
else {
pa_log("Failed to write data to FIFO: %s",
pa_cstrerror(errno));
diff --git a/src/modules/module-pipe-source.c b/src/modules/module-pipe-source.c
index 74ec0551a..32b35c163 100644
--- a/src/modules/module-pipe-source.c
+++ b/src/modules/module-pipe-source.c
@@ -155,9 +155,7 @@ static void thread_func(void *userdata) {
if (l < 0) {
-if (errno == EINTR)
-continue;
-else if (errno != EAGAIN) {
+if (errno != EAGAIN) {
pa_log("Failed to read data from FIFO: %s",
pa_cstrerror(errno));
goto fail;
}
diff --git a/src/modules/module-solaris.c b/src/modules/module-solaris.c
index 038aca114..ec9eb875f
