All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
@ 2018-05-14 23:56 Inga Stotland
  2018-05-16 12:47 ` Luiz Augusto von Dentz
  2018-05-16 13:35 ` Marcel Holtmann
  0 siblings, 2 replies; 8+ messages in thread
From: Inga Stotland @ 2018-05-14 23:56 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Inga Stotland

---
 Makefile.am         |   6 +-
 src/shared/io-ell.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+), 1 deletion(-)
 create mode 100644 src/shared/io-ell.c

diff --git a/Makefile.am b/Makefile.am
index 9c3c17139..322706fad 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -95,7 +95,8 @@ gdbus_libgdbus_internal_la_SOURCES = gdbus/gdbus.h \
 				gdbus/mainloop.c gdbus/watch.c \
 				gdbus/object.c gdbus/client.c gdbus/polkit.c
 
-noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la
+noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la \
+				src/libshared-ell.la
 
 shared_sources = src/shared/io.h src/shared/timeout.h \
 			src/shared/queue.h src/shared/queue.c \
@@ -135,6 +136,9 @@ src_libshared_mainloop_la_SOURCES = $(shared_sources) \
 				src/shared/timeout-mainloop.c \
 				src/shared/mainloop.h src/shared/mainloop.c
 
+src_libshared_ell_la_SOURCES = $(shared_sources) \
+				src/shared/io-ell.c
+
 attrib_sources = attrib/att.h attrib/att-database.h attrib/att.c \
 		attrib/gatt.h attrib/gatt.c \
 		attrib/gattrib.h attrib/gattrib.c \
diff --git a/src/shared/io-ell.c b/src/shared/io-ell.c
new file mode 100644
index 000000000..a885b935a
--- /dev/null
+++ b/src/shared/io-ell.c
@@ -0,0 +1,155 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <errno.h>
+#include <sys/socket.h>
+
+#include <ell/ell.h>
+
+#include "src/shared/io.h"
+
+struct io {
+	struct l_io *l_io;
+};
+
+struct io *io_new(int fd)
+{
+	struct io *io;
+	struct l_io *l_io;
+
+	if (fd < 0)
+		return NULL;
+
+	io = l_new(struct io, 1);
+	if (!io)
+		return NULL;
+
+	l_io = l_io_new(fd);
+	if (!io) {
+		l_free(io);
+		return NULL;
+	}
+
+	io->l_io = l_io;
+
+	return io;
+}
+
+void io_destroy(struct io *io)
+{
+	if (!io)
+		return;
+
+	if (io->l_io)
+		l_io_destroy(io->l_io);
+
+	l_free(io);
+}
+
+int io_get_fd(struct io *io)
+{
+	if (!io || !io->l_io)
+		return -ENOTCONN;
+
+	return l_io_get_fd(io->l_io);
+}
+
+bool io_set_close_on_destroy(struct io *io, bool do_close)
+{
+	if (!io || !io->l_io)
+		return false;
+
+	return l_io_set_close_on_destroy(io->l_io, do_close);
+}
+
+bool io_set_read_handler(struct io *io, io_callback_func_t callback,
+				void *user_data, io_destroy_func_t destroy)
+{
+	if (!io || !io->l_io)
+		return false;
+
+	return l_io_set_read_handler(io->l_io, (l_io_read_cb_t) callback,
+							user_data, destroy);
+}
+
+bool io_set_write_handler(struct io *io, io_callback_func_t callback,
+				void *user_data, io_destroy_func_t destroy)
+{
+	if (!io || !io->l_io)
+		return false;
+
+	return l_io_set_write_handler(io->l_io, (l_io_write_cb_t) callback,
+							user_data, destroy);
+}
+
+bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
+				void *user_data, io_destroy_func_t destroy)
+{
+	if (!io || !io->l_io)
+		return false;
+
+	return l_io_set_disconnect_handler(io->l_io,
+						(l_io_disconnect_cb_t) callback,
+							user_data, destroy);
+}
+
+ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
+{
+	ssize_t ret;
+	int fd;
+
+	if (!io || !io->l_io)
+		return -ENOTCONN;
+
+	fd = l_io_get_fd(io->l_io);
+	if (fd < 0)
+		return -ENOTCONN;
+
+	do {
+		ret = writev(fd, iov, iovcnt);
+	} while (ret < 0 && errno == EINTR);
+
+	if (ret < 0)
+		return -errno;
+
+	return ret;
+}
+
+bool io_shutdown(struct io *io)
+{
+	int fd;
+
+	if (!io || !io->l_io)
+		return false;
+
+	fd = l_io_get_fd(io->l_io);
+	if (fd < 0)
+		return false;
+
+	return shutdown(fd, SHUT_RDWR) == 0;
+}
-- 
2.14.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-14 23:56 [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling Inga Stotland
@ 2018-05-16 12:47 ` Luiz Augusto von Dentz
  2018-05-16 13:35 ` Marcel Holtmann
  1 sibling, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2018-05-16 12:47 UTC (permalink / raw)
  To: Stotland, Inga; +Cc: linux-bluetooth

Hi Inga,

On Tue, May 15, 2018 at 2:59 AM Inga Stotland <inga.stotland@intel.com>
wrote:

> ---
>   Makefile.am         |   6 +-
>   src/shared/io-ell.c | 155
++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 160 insertions(+), 1 deletion(-)
>   create mode 100644 src/shared/io-ell.c

> diff --git a/Makefile.am b/Makefile.am
> index 9c3c17139..322706fad 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -95,7 +95,8 @@ gdbus_libgdbus_internal_la_SOURCES = gdbus/gdbus.h \
>                                  gdbus/mainloop.c gdbus/watch.c \
>                                  gdbus/object.c gdbus/client.c
gdbus/polkit.c

> -noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la
> +noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la \
> +                               src/libshared-ell.la

>   shared_sources = src/shared/io.h src/shared/timeout.h \
>                          src/shared/queue.h src/shared/queue.c \
> @@ -135,6 +136,9 @@ src_libshared_mainloop_la_SOURCES = $(shared_sources)
\
>                                  src/shared/timeout-mainloop.c \
>                                  src/shared/mainloop.h
src/shared/mainloop.c

> +src_libshared_ell_la_SOURCES = $(shared_sources) \
> +                               src/shared/io-ell.c
> +
>   attrib_sources = attrib/att.h attrib/att-database.h attrib/att.c \
>                  attrib/gatt.h attrib/gatt.c \
>                  attrib/gattrib.h attrib/gattrib.c \
> diff --git a/src/shared/io-ell.c b/src/shared/io-ell.c
> new file mode 100644
> index 000000000..a885b935a
> --- /dev/null
> +++ b/src/shared/io-ell.c
> @@ -0,0 +1,155 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2018  Intel Corporation. All rights reserved.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/socket.h>
> +
> +#include <ell/ell.h>
> +
> +#include "src/shared/io.h"
> +
> +struct io {
> +       struct l_io *l_io;
> +};
> +
> +struct io *io_new(int fd)
> +{
> +       struct io *io;
> +       struct l_io *l_io;
> +
> +       if (fd < 0)
> +               return NULL;
> +
> +       io = l_new(struct io, 1);
> +       if (!io)
> +               return NULL;
> +
> +       l_io = l_io_new(fd);
> +       if (!io) {
> +               l_free(io);
> +               return NULL;
> +       }
> +
> +       io->l_io = l_io;
> +
> +       return io;
> +}
> +
> +void io_destroy(struct io *io)
> +{
> +       if (!io)
> +               return;
> +
> +       if (io->l_io)
> +               l_io_destroy(io->l_io);
> +
> +       l_free(io);
> +}
> +
> +int io_get_fd(struct io *io)
> +{
> +       if (!io || !io->l_io)
> +               return -ENOTCONN;
> +
> +       return l_io_get_fd(io->l_io);
> +}
> +
> +bool io_set_close_on_destroy(struct io *io, bool do_close)
> +{
> +       if (!io || !io->l_io)
> +               return false;
> +
> +       return l_io_set_close_on_destroy(io->l_io, do_close);
> +}
> +
> +bool io_set_read_handler(struct io *io, io_callback_func_t callback,
> +                               void *user_data, io_destroy_func_t
destroy)
> +{
> +       if (!io || !io->l_io)
> +               return false;
> +
> +       return l_io_set_read_handler(io->l_io, (l_io_read_cb_t) callback,
> +                                                       user_data,
destroy);
> +}
> +
> +bool io_set_write_handler(struct io *io, io_callback_func_t callback,
> +                               void *user_data, io_destroy_func_t
destroy)
> +{
> +       if (!io || !io->l_io)
> +               return false;
> +
> +       return l_io_set_write_handler(io->l_io, (l_io_write_cb_t)
callback,
> +                                                       user_data,
destroy);
> +}
> +
> +bool io_set_disconnect_handler(struct io *io, io_callback_func_t
callback,
> +                               void *user_data, io_destroy_func_t
destroy)
> +{
> +       if (!io || !io->l_io)
> +               return false;
> +
> +       return l_io_set_disconnect_handler(io->l_io,
> +                                               (l_io_disconnect_cb_t)
callback,
> +                                                       user_data,
destroy);
> +}
> +
> +ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> +{
> +       ssize_t ret;
> +       int fd;
> +
> +       if (!io || !io->l_io)
> +               return -ENOTCONN;
> +
> +       fd = l_io_get_fd(io->l_io);
> +       if (fd < 0)
> +               return -ENOTCONN;
> +
> +       do {
> +               ret = writev(fd, iov, iovcnt);
> +       } while (ret < 0 && errno == EINTR);
> +
> +       if (ret < 0)
> +               return -errno;
> +
> +       return ret;
> +}
> +
> +bool io_shutdown(struct io *io)
> +{
> +       int fd;
> +
> +       if (!io || !io->l_io)
> +               return false;
> +
> +       fd = l_io_get_fd(io->l_io);
> +       if (fd < 0)
> +               return false;
> +
> +       return shutdown(fd, SHUT_RDWR) == 0;
> +}
> --
> 2.14.3

Applied, thanks.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-14 23:56 [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling Inga Stotland
  2018-05-16 12:47 ` Luiz Augusto von Dentz
@ 2018-05-16 13:35 ` Marcel Holtmann
  2018-05-16 14:39   ` Denis Kenzior
  1 sibling, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2018-05-16 13:35 UTC (permalink / raw)
  To: Inga Stotland; +Cc: linux-bluetooth

Hi Inga,

> ---
> Makefile.am         |   6 +-
> src/shared/io-ell.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 160 insertions(+), 1 deletion(-)
> create mode 100644 src/shared/io-ell.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 9c3c17139..322706fad 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -95,7 +95,8 @@ gdbus_libgdbus_internal_la_SOURCES = gdbus/gdbus.h \
> 				gdbus/mainloop.c gdbus/watch.c \
> 				gdbus/object.c gdbus/client.c gdbus/polkit.c
> 
> -noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la
> +noinst_LTLIBRARIES += src/libshared-glib.la src/libshared-mainloop.la \
> +				src/libshared-ell.la
> 
> shared_sources = src/shared/io.h src/shared/timeout.h \
> 			src/shared/queue.h src/shared/queue.c \
> @@ -135,6 +136,9 @@ src_libshared_mainloop_la_SOURCES = $(shared_sources) \
> 				src/shared/timeout-mainloop.c \
> 				src/shared/mainloop.h src/shared/mainloop.c
> 
> +src_libshared_ell_la_SOURCES = $(shared_sources) \
> +				src/shared/io-ell.c
> +
> attrib_sources = attrib/att.h attrib/att-database.h attrib/att.c \
> 		attrib/gatt.h attrib/gatt.c \
> 		attrib/gattrib.h attrib/gattrib.c \
> diff --git a/src/shared/io-ell.c b/src/shared/io-ell.c
> new file mode 100644
> index 000000000..a885b935a
> --- /dev/null
> +++ b/src/shared/io-ell.c
> @@ -0,0 +1,155 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2018  Intel Corporation. All rights reserved.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/socket.h>
> +
> +#include <ell/ell.h>
> +
> +#include "src/shared/io.h"
> +
> +struct io {
> +	struct l_io *l_io;
> +};
> +
> +struct io *io_new(int fd)
> +{
> +	struct io *io;
> +	struct l_io *l_io;
> +
> +	if (fd < 0)
> +		return NULL;
> +
> +	io = l_new(struct io, 1);
> +	if (!io)
> +		return NULL;
> +
> +	l_io = l_io_new(fd);
> +	if (!io) {
> +		l_free(io);
> +		return NULL;
> +	}
> +
> +	io->l_io = l_io;
> +
> +	return io;
> +}
> +
> +void io_destroy(struct io *io)
> +{
> +	if (!io)
> +		return;
> +
> +	if (io->l_io)
> +		l_io_destroy(io->l_io);
> +
> +	l_free(io);
> +}
> +
> +int io_get_fd(struct io *io)
> +{
> +	if (!io || !io->l_io)
> +		return -ENOTCONN;
> +
> +	return l_io_get_fd(io->l_io);
> +}
> +
> +bool io_set_close_on_destroy(struct io *io, bool do_close)
> +{
> +	if (!io || !io->l_io)
> +		return false;
> +
> +	return l_io_set_close_on_destroy(io->l_io, do_close);
> +}
> +
> +bool io_set_read_handler(struct io *io, io_callback_func_t callback,
> +				void *user_data, io_destroy_func_t destroy)
> +{
> +	if (!io || !io->l_io)
> +		return false;
> +
> +	return l_io_set_read_handler(io->l_io, (l_io_read_cb_t) callback,
> +							user_data, destroy);
> +}
> +
> +bool io_set_write_handler(struct io *io, io_callback_func_t callback,
> +				void *user_data, io_destroy_func_t destroy)
> +{
> +	if (!io || !io->l_io)
> +		return false;
> +
> +	return l_io_set_write_handler(io->l_io, (l_io_write_cb_t) callback,
> +							user_data, destroy);
> +}
> +
> +bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> +				void *user_data, io_destroy_func_t destroy)
> +{
> +	if (!io || !io->l_io)
> +		return false;
> +
> +	return l_io_set_disconnect_handler(io->l_io,
> +						(l_io_disconnect_cb_t) callback,
> +							user_data, destroy);
> +}
> +
> +ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> +{
> +	ssize_t ret;
> +	int fd;
> +
> +	if (!io || !io->l_io)
> +		return -ENOTCONN;
> +
> +	fd = l_io_get_fd(io->l_io);
> +	if (fd < 0)
> +		return -ENOTCONN;
> +
> +	do {
> +		ret = writev(fd, iov, iovcnt);
> +	} while (ret < 0 && errno == EINTR);

explain this one to me. Or maybe Luiz should explain it since he introduced this.

Regards

Marcel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-16 13:35 ` Marcel Holtmann
@ 2018-05-16 14:39   ` Denis Kenzior
  2018-05-17  8:01     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Denis Kenzior @ 2018-05-16 14:39 UTC (permalink / raw)
  To: Marcel Holtmann, Inga Stotland; +Cc: linux-bluetooth

Hi,

>> +
>> +	do {
>> +		ret = writev(fd, iov, iovcnt);
>> +	} while (ret < 0 && errno == EINTR);
> 
> explain this one to me. Or maybe Luiz should explain it since he introduced this.
> 

I'm curious why not use TEMP_FAILURE_RETRY macro?  ell already uses that 
for clarity.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-16 14:39   ` Denis Kenzior
@ 2018-05-17  8:01     ` Luiz Augusto von Dentz
  2018-05-17 15:01       ` Denis Kenzior
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2018-05-17  8:01 UTC (permalink / raw)
  To: Denis Kenzior; +Cc: Marcel Holtmann, Stotland, Inga, linux-bluetooth

Hi,
On Wed, May 16, 2018 at 5:42 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi,

> >> +
> >> +    do {
> >> +            ret = writev(fd, iov, iovcnt);
> >> +    } while (ret < 0 && errno == EINTR);
> >
> > explain this one to me. Or maybe Luiz should explain it since he
introduced this.
> >

> I'm curious why not use TEMP_FAILURE_RETRY macro?  ell already uses that
> for clarity.

Szymon was actually suggesting that we should have something similar in
ell, e.g. l_io_send, it would be nice if that would handle iovec similarly
to how is done here but perhaps using the TEMP_FAILURE_RETRY as suggested
by Denis.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-17  8:01     ` Luiz Augusto von Dentz
@ 2018-05-17 15:01       ` Denis Kenzior
  2018-05-17 16:03         ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Denis Kenzior @ 2018-05-17 15:01 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, Stotland, Inga, linux-bluetooth

Hi Luiz,

On 05/17/2018 03:01 AM, Luiz Augusto von Dentz wrote:
> Hi,
> On Wed, May 16, 2018 at 5:42 PM Denis Kenzior <denkenz@gmail.com> wrote:
> 
>> Hi,
> 
>>>> +
>>>> +    do {
>>>> +            ret = writev(fd, iov, iovcnt);
>>>> +    } while (ret < 0 && errno == EINTR);
>>>
>>> explain this one to me. Or maybe Luiz should explain it since he
> introduced this.
>>>
> 
>> I'm curious why not use TEMP_FAILURE_RETRY macro?  ell already uses that
>> for clarity.
> 
> Szymon was actually suggesting that we should have something similar in
> ell, e.g. l_io_send, it would be nice if that would handle iovec similarly
> to how is done here but perhaps using the TEMP_FAILURE_RETRY as suggested
> by Denis.
> 

I would rather not do that actually.  It is already weird that you guys 
have io_send but not io_recv.  And since io is not only used with 
streams / files, but with datagram sockets as well, we would have to add 
a bunch of methods for symmetry.  Not to mention that somehow 'send' 
uses iovecs and the convention inside ell for this is to suffix the 
method with a 'v'.

Also, just a nitpick, but io_send should really be io_writev.   You're 
just confusing every hardcore UNIX user here since a vectorized send is 
'sendmsg'.

By the way, correct me if I'm wrong, but I think there is zero need for 
TEMP_FAILURE_RETRY inside BlueZ since it uses signalfd and non-blocking 
IO anyway.  The only reason ell uses this macro is because we can't 
assume how signals are used in the enclosing application.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-17 15:01       ` Denis Kenzior
@ 2018-05-17 16:03         ` Luiz Augusto von Dentz
  2018-05-17 16:29           ` Denis Kenzior
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2018-05-17 16:03 UTC (permalink / raw)
  To: Denis Kenzior; +Cc: Marcel Holtmann, Stotland, Inga, linux-bluetooth

Hi Denis,

On Thu, May 17, 2018 at 6:01 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Luiz,
>
> On 05/17/2018 03:01 AM, Luiz Augusto von Dentz wrote:
>>
>> Hi,
>> On Wed, May 16, 2018 at 5:42 PM Denis Kenzior <denkenz@gmail.com> wrote:
>>
>>> Hi,
>>
>>
>>>>> +
>>>>> +    do {
>>>>> +            ret = writev(fd, iov, iovcnt);
>>>>> +    } while (ret < 0 && errno == EINTR);
>>>>
>>>>
>>>> explain this one to me. Or maybe Luiz should explain it since he
>>
>> introduced this.
>>>>
>>>>
>>
>>> I'm curious why not use TEMP_FAILURE_RETRY macro?  ell already uses that
>>> for clarity.
>>
>>
>> Szymon was actually suggesting that we should have something similar in
>> ell, e.g. l_io_send, it would be nice if that would handle iovec similarly
>> to how is done here but perhaps using the TEMP_FAILURE_RETRY as suggested
>> by Denis.
>>
>
> I would rather not do that actually.  It is already weird that you guys have
> io_send but not io_recv.  And since io is not only used with streams /
> files, but with datagram sockets as well, we would have to add a bunch of
> methods for symmetry.  Not to mention that somehow 'send' uses iovecs and
> the convention inside ell for this is to suffix the method with a 'v'.

Ive thought of adding io_recv, it just that it was not necessary by
anyone currently, anyway I guess having _v alternative makes ell play
nice if the application is using iovec, or you are saying that with
ell the application would have to use the fd directly and handle
errors?

> Also, just a nitpick, but io_send should really be io_writev.   You're just
> confusing every hardcore UNIX user here since a vectorized send is
> 'sendmsg'.

Yep, changing it is not a problem though since it just an internal API.

> By the way, correct me if I'm wrong, but I think there is zero need for
> TEMP_FAILURE_RETRY inside BlueZ since it uses signalfd and non-blocking IO
> anyway.  The only reason ell uses this macro is because we can't assume how
> signals are used in the enclosing application.

Usually, BlueZ tools would use signalfd, including the daemon, we
could perhaps remove the handling of EINTR if we could detect if
signalfd is in fact being used but I don't know if that is possible,
so better be save than sorry.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling
  2018-05-17 16:03         ` Luiz Augusto von Dentz
@ 2018-05-17 16:29           ` Denis Kenzior
  0 siblings, 0 replies; 8+ messages in thread
From: Denis Kenzior @ 2018-05-17 16:29 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, Stotland, Inga, linux-bluetooth

Hi Luiz,

>> I would rather not do that actually.  It is already weird that you guys have
>> io_send but not io_recv.  And since io is not only used with streams /
>> files, but with datagram sockets as well, we would have to add a bunch of
>> methods for symmetry.  Not to mention that somehow 'send' uses iovecs and
>> the convention inside ell for this is to suffix the method with a 'v'.
> 
> Ive thought of adding io_recv, it just that it was not necessary by
> anyone currently, anyway I guess having _v alternative makes ell play
> nice if the application is using iovec, or you are saying that with
> ell the application would have to use the fd directly and handle
> errors?

Right, that's the intent of the ell api.  We only handle the mainloop 
integration with io.  The actual send/recv/write/read/etc should be 
handled outside l_io to keep the l_io API minimal.  There are too many 
variations between socket types, streams, etc.  Most of our projects 
bypassed the weird GLib I/O functions (with its buffering semantics, 
etc) and just invoked the system calls directly anyway.

>> By the way, correct me if I'm wrong, but I think there is zero need for
>> TEMP_FAILURE_RETRY inside BlueZ since it uses signalfd and non-blocking IO
>> anyway.  The only reason ell uses this macro is because we can't assume how
>> signals are used in the enclosing application.
> 
> Usually, BlueZ tools would use signalfd, including the daemon, we
> could perhaps remove the handling of EINTR if we could detect if
> signalfd is in fact being used but I don't know if that is possible,
> so better be save than sorry.

So I personally wouldn't introduce quasi-ell API that is not mirrored 
inside ell proper.  That will just make your job harder in the future. 
And if the caller of io_send just handles the writev invocation you can 
know exactly if TEMP_FAILURE_RETRY is needed or not.

Ideally this file should not exist.  You might get away with just 
(struct l_io *) io or vice-versa.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-05-17 16:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 23:56 [PATCH BlueZ] shared/io-ell: Add support for ELL based IO handling Inga Stotland
2018-05-16 12:47 ` Luiz Augusto von Dentz
2018-05-16 13:35 ` Marcel Holtmann
2018-05-16 14:39   ` Denis Kenzior
2018-05-17  8:01     ` Luiz Augusto von Dentz
2018-05-17 15:01       ` Denis Kenzior
2018-05-17 16:03         ` Luiz Augusto von Dentz
2018-05-17 16:29           ` Denis Kenzior

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.