[PATCH 0/2] io error/disconnect fixes

[PATCH 0/2] io error/disconnect fixes

[Inga Stotland]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

This patchset addresses cases when an error condition is detected in
io_callback() and, as a result, io is closed (disconnected), socket is
closed and disconnect hanler (if set) is called.

The current implementation closes the socket first and only then informs the
client that a disconnect has occured. At this point socket error condition,
e.g., ECONNREFUSED, is lost and this potentially prevents the client to take
different course of action that may have depended on the error.

Also, a disconnect callback is removed from the l_io_destroy(). The reasoning
here is that since l_io_destroy is an API function and is invoked by a client,
there is no reason to inform that same clinet that the IO has been disconnected.
This allows avoiding some circuitous logic in ell/io.c

Inga Stotland (2):
  io: Do not call disconnect handler in destroyer
  io: Allow socket error to be retrieved on disconnect

 ell/io.c | 33 ++++++++++-----------------------
 1 file changed, 10 insertions(+), 23 deletions(-)

-- 
2.26.2

[PATCH 1/2] io: Do not call disconnect handler in destroyer

[Inga Stotland]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

When a client is calling l_io_destroy() there is no need to inform
that same client that io has been disconnected.

Removing the disconnect callback from l_io_destroy() eliminates the
need to zero out and preserve disconnect handler data when invoking
disconnect handler in io_closed().

Also, rename static function io_closed() to io_disconnected() to
reflect the actual functionality.
---
 ell/io.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/ell/io.c b/ell/io.c
index 41ff6dd..1fa5f0b 100644
--- a/ell/io.c
+++ b/ell/io.c
@@ -86,25 +86,13 @@ static void io_cleanup(void *user_data)
 	io->fd = -1;
 }
 
-static void io_closed(struct l_io *io)
+static void io_disconnected(struct l_io *io)
 {
-	/*
-	 * Save off copies of disconnect_handler, disconnect_destroy
-	 * and disconnect_data in case the handler calls io_destroy
-	 */
-	l_io_disconnect_cb_t handler = io->disconnect_handler;
-	l_io_destroy_cb_t destroy = io->disconnect_destroy;
-	void *disconnect_data = io->disconnect_data;
-
-	io->disconnect_handler = NULL;
-	io->disconnect_destroy = NULL;
-	io->disconnect_data = NULL;
-
-	if (handler)
-		handler(io, disconnect_data);
-
-	if (destroy)
-		destroy(disconnect_data);
+	if (io->disconnect_handler)
+		io->disconnect_handler(io, io->disconnect_data);
+
+	if (io->disconnect_destroy)
+		io->disconnect_destroy(io->disconnect_data);
 }
 
 static void io_callback(int fd, uint32_t events, void *user_data)
@@ -115,7 +103,7 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 		l_util_debug(io->debug_handler, io->debug_data,
 						"disconnect event <%p>", io);
 		watch_remove(io->fd, !io->close_on_destroy);
-		io_closed(io);
+		io_disconnected(io);
 		return;
 	}
 
@@ -136,7 +124,7 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 			if (watch_modify(io->fd, io->events, false) == -EBADF) {
 				io->close_on_destroy = false;
 				watch_clear(io->fd);
-				io_closed(io);
+				io_disconnected(io);
 				return;
 			}
 		}
@@ -159,7 +147,7 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 			if (watch_modify(io->fd, io->events, false) == -EBADF) {
 				io->close_on_destroy = false;
 				watch_clear(io->fd);
-				io_closed(io);
+				io_disconnected(io);
 				return;
 			}
 		}
@@ -211,8 +199,6 @@ LIB_EXPORT void l_io_destroy(struct l_io *io)
 	if (io->fd != -1)
 		watch_remove(io->fd, !io->close_on_destroy);
 
-	io_closed(io);
-
 	if (io->debug_destroy)
 		io->debug_destroy(io->debug_data);
 
-- 
2.26.2

[PATCH 2/2] io: Allow socket error to be retrieved on disconnect

[Inga Stotland]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

When encountering an error condition, first invoke disconnect handler
if it is set, and only then clear/remove the watch.

This allows the client to retrieve the the actual code for the socket
error (via getsockopt()). Otherwise, if the socket is closed
prior to invoking disconnect handler, the error code is lost.
---
 ell/io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ell/io.c b/ell/io.c
index 1fa5f0b..169dfb2 100644
--- a/ell/io.c
+++ b/ell/io.c
@@ -102,8 +102,9 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 	if (unlikely(events & (EPOLLERR | EPOLLHUP))) {
 		l_util_debug(io->debug_handler, io->debug_data,
 						"disconnect event <%p>", io);
-		watch_remove(io->fd, !io->close_on_destroy);
+
 		io_disconnected(io);
+		watch_remove(io->fd, !io->close_on_destroy);
 		return;
 	}
 
@@ -123,8 +124,8 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 
 			if (watch_modify(io->fd, io->events, false) == -EBADF) {
 				io->close_on_destroy = false;
-				watch_clear(io->fd);
 				io_disconnected(io);
+				watch_clear(io->fd);
 				return;
 			}
 		}
@@ -146,8 +147,8 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 
 			if (watch_modify(io->fd, io->events, false) == -EBADF) {
 				io->close_on_destroy = false;
-				watch_clear(io->fd);
 				io_disconnected(io);
+				watch_clear(io->fd);
 				return;
 			}
 		}
-- 
2.26.2

Re: [PATCH 1/2] io: Do not call disconnect handler in destroyer

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

Hi Inga,

On 10/12/20 5:13 PM, Inga Stotland wrote:
> When a client is calling l_io_destroy() there is no need to inform
> that same client that io has been disconnected.

Hmm... the client invoking l_io_destroy might be different from the one that set 
the disconnect handler?  Also, I think this might break some clients. 
ofono.git/plugins/mbim.c for example seems like it is affected due to how 
mbim_device is implemented...

> 
> Removing the disconnect callback from l_io_destroy() eliminates the
> need to zero out and preserve disconnect handler data when invoking
> disconnect handler in io_closed().
> 
> Also, rename static function io_closed() to io_disconnected() to
> reflect the actual functionality.

Is anything really broken due to the current behavior?

> ---
>   ell/io.c | 32 +++++++++-----------------------
>   1 file changed, 9 insertions(+), 23 deletions(-)
> 

Regards,
-Denis

Re: [PATCH 1/2] io: Do not call disconnect handler in destroyer

[Stotland, Inga]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

Hi Denis,

On Tue, 2020-10-13 at 13:34 -0500, Denis Kenzior wrote:

Hi Inga,


On 10/12/20 5:13 PM, Inga Stotland wrote:

When a client is calling l_io_destroy() there is no need to inform

that same client that io has been disconnected.


Hmm... the client invoking l_io_destroy might be different from the one that set

the disconnect handler?  Also, I think this might break some clients.

ofono.git/plugins/mbim.c for example seems like it is affected due to how

mbim_device is implemented...



Removing the disconnect callback from l_io_destroy() eliminates the

need to zero out and preserve disconnect handler data when invoking

disconnect handler in io_closed().


Also, rename static function io_closed() to io_disconnected() to

reflect the actual functionality.


Is anything really broken due to the current behavior?

I guess not. I was just thrown off by the comment "in case the handler calls io_destroy" in io_closed().
Let's disregard this patch.



---

  ell/io.c | 32 +++++++++-----------------------

  1 file changed, 9 insertions(+), 23 deletions(-)



Regards,

-Denis

[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 2373 bytes --]