If the FD is both readable and writable and the read callback closes the connection (and frees the surrounding structure) we shouldn't call the write callback (or check anything else in the read fd).
With this patch callback functions can return -EBADFD if they don't want the FD to be handled any more. --- src/write_queue.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/write_queue.c b/src/write_queue.c index cef40f8..d17493d 100644 --- a/src/write_queue.c +++ b/src/write_queue.c @@ -21,8 +21,15 @@ * */
+#include <errno.h> #include <osmocom/core/write_queue.h>
+#define HANDLE_BAD_FD(rc, label) \ + do { \ + if (rc == -EBADFD) \ + goto label; \ + } while (0); + /*! \addtogroup write_queue * @{ */ @@ -39,14 +46,19 @@ int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what) { struct osmo_wqueue *queue; + int rc;
queue = container_of(fd, struct osmo_wqueue, bfd);
- if (what & BSC_FD_READ) - queue->read_cb(fd); + if (what & BSC_FD_READ) { + rc = queue->read_cb(fd); + HANDLE_BAD_FD(rc, err_badfd); + }
- if (what & BSC_FD_EXCEPT) - queue->except_cb(fd); + if (what & BSC_FD_EXCEPT) { + rc = queue->except_cb(fd); + HANDLE_BAD_FD(rc, err_badfd); + }
if (what & BSC_FD_WRITE) { struct msgb *msg; @@ -58,16 +70,21 @@ int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what) --queue->current_length;
msg = msgb_dequeue(&queue->msg_queue); - queue->write_cb(fd, msg); + rc = queue->write_cb(fd, msg); msgb_free(msg);
+ HANDLE_BAD_FD(rc, err_badfd); + if (!llist_empty(&queue->msg_queue)) fd->when |= BSC_FD_WRITE; } }
return 0; +err_badfd: + return rc; } +#undef HANDLE_BAD_FD
/*! \brief Initialize a \ref osmo_wqueue structure * \param[in] queue Write queue to operate on
If the read callback closes the connection conn is already freed so we can't derefernce it. Instead return -EBADFD in the read function if it closed the connection and check for that. --- src/vty/telnet_interface.c | 3 +-- src/vty/vty.c | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/vty/telnet_interface.c b/src/vty/telnet_interface.c index 32ab6be..0a04d15 100644 --- a/src/vty/telnet_interface.c +++ b/src/vty/telnet_interface.c @@ -120,7 +120,7 @@ static int client_data(struct osmo_fd *fd, unsigned int what) }
/* vty might have been closed from vithin vty_read() */ - if (!conn->vty) + if (rc == -EBADFD) return rc;
if (what & BSC_FD_WRITE) { @@ -193,7 +193,6 @@ void vty_event(enum event event, int sock, struct vty *vty) break; case VTY_CLOSED: /* vty layer is about to free() vty */ - connection->vty = NULL; telnet_close_client(bfd); break; default: diff --git a/src/vty/vty.c b/src/vty/vty.c index 8bfc35c..fc86bdf 100644 --- a/src/vty/vty.c +++ b/src/vty/vty.c @@ -1432,9 +1432,10 @@ int vty_read(struct vty *vty) }
/* Check status. */ - if (vty->status == VTY_CLOSE) + if (vty->status == VTY_CLOSE) { vty_close(vty); - else { + return -EBADFD; + } else { vty_event(VTY_WRITE, vty_sock, vty); vty_event(VTY_READ, vty_sock, vty); }
On Wed, 2014-06-04 at 17:58, Holger Hans Peter Freyther wrote:
On Wed, May 21, 2014 at 03:08:18PM +0200, Daniel Willmann wrote:
return 0; +err_badfd:
- return rc;
hi, why do you want to change the return value here? Why is the return 0 not good enough here?
You're right, it would be. In fact, the return value of this callback is not checked in osmo_select_main() at all (and there doesn't seem to be any use for it in that place either). I'll update the patch to return 0 in both cases.
Regards, Daniel
From: Daniel Willmann dwillmann@sysmocom.de
If the FD is both readable and writable and the read callback closes the connection (and frees the surrounding structure) we shouldn't call the write callback (or check anything else in the read fd).
With this patch callback functions can return -EBADFD if they don't want the FD to be handled any more. --- src/write_queue.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/write_queue.c b/src/write_queue.c index cef40f8..dcc0469 100644 --- a/src/write_queue.c +++ b/src/write_queue.c @@ -21,8 +21,15 @@ * */
+#include <errno.h> #include <osmocom/core/write_queue.h>
+#define HANDLE_BAD_FD(rc, label) \ + do { \ + if (rc == -EBADFD) \ + goto label; \ + } while (0); + /*! \addtogroup write_queue * @{ */ @@ -39,14 +46,19 @@ int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what) { struct osmo_wqueue *queue; + int rc;
queue = container_of(fd, struct osmo_wqueue, bfd);
- if (what & BSC_FD_READ) - queue->read_cb(fd); + if (what & BSC_FD_READ) { + rc = queue->read_cb(fd); + HANDLE_BAD_FD(rc, err_badfd); + }
- if (what & BSC_FD_EXCEPT) - queue->except_cb(fd); + if (what & BSC_FD_EXCEPT) { + rc = queue->except_cb(fd); + HANDLE_BAD_FD(rc, err_badfd); + }
if (what & BSC_FD_WRITE) { struct msgb *msg; @@ -58,16 +70,21 @@ int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what) --queue->current_length;
msg = msgb_dequeue(&queue->msg_queue); - queue->write_cb(fd, msg); + rc = queue->write_cb(fd, msg); msgb_free(msg);
+ HANDLE_BAD_FD(rc, err_badfd); + if (!llist_empty(&queue->msg_queue)) fd->when |= BSC_FD_WRITE; } }
+err_badfd: + /* Return value is not checked in osmo_select_main() */ return 0; } +#undef HANDLE_BAD_FD
/*! \brief Initialize a \ref osmo_wqueue structure * \param[in] queue Write queue to operate on
On Tue, Jun 10, 2014 at 10:02:24AM +0200, Daniel Willmann wrote:
From: Daniel Willmann dwillmann@sysmocom.de
If the FD is both readable and writable and the read callback closes the connection (and frees the surrounding structure) we shouldn't call the write callback (or check anything else in the read fd).
With this patch callback functions can return -EBADFD if they don't want the FD to be handled any more.
src/write_queue.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/write_queue.c b/src/write_queue.c index cef40f8..dcc0469 100644 --- a/src/write_queue.c +++ b/src/write_queue.c @@ -21,8 +21,15 @@
*/
+#include <errno.h> #include <osmocom/core/write_queue.h>
+#define HANDLE_BAD_FD(rc, label) \
- do { \
if (rc == -EBADFD) \goto label; \- } while (0);
Do we really get anything good with this macro? This checking is only required in three places in this patch.
On Tue, Jun 10, 2014 at 10:27:52AM +0200, Pablo Neira Ayuso wrote:
+#define HANDLE_BAD_FD(rc, label) \
- do { \
if (rc == -EBADFD) \goto label; \- } while (0);
Do we really get anything good with this macro? This checking is only required in three places in this patch.
What is the argument against having this macro? readability?
-
Daniel Willmann -
Daniel Willmann -
Holger Hans Peter Freyther -
Pablo Neira Ayuso