DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B984B2083E
MIME-Version: 1.0
References: <20181115115324.10215-1-gbenhaim@augury.com>
In-Reply-To: <20181115115324.10215-1-gbenhaim@augury.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 15 Nov 2018 14:37:48 +0200
Message-ID: <CABBYNZKkrddaFypEA0FGW8CHAH3bf-atNKh2+6scLKC3ZPSfQQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ] shared/io-mainloop: Try to write to the pipe only
 if there are readers
To:     gbenhaim@augury.com
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Gal,
On Thu, Nov 15, 2018 at 1:55 PM Gal Ben-Haim <gbenhaim@augury.com> wrote:
>
> bluetoothd receives a SIGPIPE and terminates if writing to a pipe that
> was acquired by AcquireNotify and there are no readers. it can be
> reproduced by terminating the reader process without closing the reader
> end of the pipe.
>
> poll is used to check for a POLLRDHUP event before write and an EPIPE
> error is returned if the peer closed its end of the pipe.
> ---
>  src/shared/io-mainloop.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
> index 2306c3479..4a70449fc 100644
> --- a/src/shared/io-mainloop.c
> +++ b/src/shared/io-mainloop.c
> @@ -27,6 +27,7 @@
>
>  #include <unistd.h>
>  #include <errno.h>
> +#include <poll.h>
>  #include <sys/socket.h>
>
>  #include "src/shared/mainloop.h"
> @@ -305,7 +306,14 @@ ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
>         if (!io || io->fd < 0)
>                 return -ENOTCONN;
>
> +       struct pollfd fds[1];
> +       fds[0].fd = io->fd;
> +       fds[0].events = POLLRDHUP;
>         do {
> +               if (poll(fds, 1, 0) > 0 && fds[0].revents & POLLRDHUP > 0) {
> +                       return -EPIPE;
> +               }
> +
>                 ret = writev(io->fd, iov, iovcnt);
>         } while (ret < 0 && errno == EINTR);
>
> --
> 2.19.1

We normally do it a bit differently, we setup a write handler using
io_set_write_handler which informs when the fd is writable, otherwise
we maybe polling twice here since the user of the io may already be
using that mechanism. That said you are actually checking for HUP
which is something done io_set_disconnect_handler, which is already in
use. I suspect to really get this done cleanly we should perhaps use
MSG_NOSIGNAL when trying to write.

-- 
Luiz Augusto von Dentz