diff mbox series

[2/5] io/channel.c,io/channel-socket.c: Add yank feature

Message ID db00ff13f62fe97634132e43105b9bf63bbbf582.1589193717.git.lukasstraub2@web.de (mailing list archive)
State New, archived
Headers show
Series Introduce 'yank' oob qmp command to recover from hanging qemu | expand

Commit Message

Lukas Straub May 11, 2020, 11:14 a.m. UTC
Add qio_channel_set_yank function to channel and to channel-socket,
which will register a yank function. The yank function calls
shutdown() on the socket.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 Makefile.objs               |  1 +
 include/io/channel-socket.h |  1 +
 include/io/channel.h        | 12 ++++++++++++
 io/channel-socket.c         | 29 +++++++++++++++++++++++++++++
 io/channel.c                |  9 +++++++++
 5 files changed, 52 insertions(+)

Comments

Daniel P. Berrangé May 11, 2020, 11:51 a.m. UTC | #1
On Mon, May 11, 2020 at 01:14:41PM +0200, Lukas Straub wrote:
> Add qio_channel_set_yank function to channel and to channel-socket,
> which will register a yank function. The yank function calls
> shutdown() on the socket.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  Makefile.objs               |  1 +
>  include/io/channel-socket.h |  1 +
>  include/io/channel.h        | 12 ++++++++++++
>  io/channel-socket.c         | 29 +++++++++++++++++++++++++++++
>  io/channel.c                |  9 +++++++++
>  5 files changed, 52 insertions(+)

Assuming we want the yank feature (which I'm not entirely convinced
of), then I don't think any of this addition should exist. The
QIOChannel class already provides a "qio_channel_shutdown" method
which can be invoked. The layer above which is using the QIOChannel
should be calling this existing qio_channel_shutdown method in
response to any yank request.  The I/O layer shouldn't have any
direct dependancy on the yank feature.


Regards,
Daniel
Lukas Straub May 11, 2020, 8 p.m. UTC | #2
On Mon, 11 May 2020 12:51:46 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, May 11, 2020 at 01:14:41PM +0200, Lukas Straub wrote:
> > Add qio_channel_set_yank function to channel and to channel-socket,
> > which will register a yank function. The yank function calls
> > shutdown() on the socket.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  Makefile.objs               |  1 +
> >  include/io/channel-socket.h |  1 +
> >  include/io/channel.h        | 12 ++++++++++++
> >  io/channel-socket.c         | 29 +++++++++++++++++++++++++++++
> >  io/channel.c                |  9 +++++++++
> >  5 files changed, 52 insertions(+)  
> 
> Assuming we want the yank feature (which I'm not entirely convinced
> of), then I don't think any of this addition should exist. The
> QIOChannel class already provides a "qio_channel_shutdown" method
> which can be invoked. The layer above which is using the QIOChannel
> should be calling this existing qio_channel_shutdown method in
> response to any yank request.  The I/O layer shouldn't have any
> direct dependancy on the yank feature.

Having the code here simplifys the code in the other places.

Regards,
Lukas Straub

> 
> Regards,
> Daniel
Daniel P. Berrangé May 12, 2020, 8:52 a.m. UTC | #3
On Mon, May 11, 2020 at 10:00:42PM +0200, Lukas Straub wrote:
> On Mon, 11 May 2020 12:51:46 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Mon, May 11, 2020 at 01:14:41PM +0200, Lukas Straub wrote:
> > > Add qio_channel_set_yank function to channel and to channel-socket,
> > > which will register a yank function. The yank function calls
> > > shutdown() on the socket.
> > > 
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  Makefile.objs               |  1 +
> > >  include/io/channel-socket.h |  1 +
> > >  include/io/channel.h        | 12 ++++++++++++
> > >  io/channel-socket.c         | 29 +++++++++++++++++++++++++++++
> > >  io/channel.c                |  9 +++++++++
> > >  5 files changed, 52 insertions(+)  
> > 
> > Assuming we want the yank feature (which I'm not entirely convinced
> > of), then I don't think any of this addition should exist. The
> > QIOChannel class already provides a "qio_channel_shutdown" method
> > which can be invoked. The layer above which is using the QIOChannel
> > should be calling this existing qio_channel_shutdown method in
> > response to any yank request.  The I/O layer shouldn't have any
> > direct dependancy on the yank feature.
> 
> Having the code here simplifys the code in the other places.

This introduces a depedancy from the IO channel code into the system
emulator infra which is not desired. The goal is to keep the io/ module
isolated and as self-contained as possible.

I don't think it makes a significant difference to the yank code to
keep it out of the io layer, and simply call the QIOChannel objects
via their existing public API.

Regards,
Daniel
diff mbox series

Patch

diff --git a/Makefile.objs b/Makefile.objs
index a7c967633a..889115775c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -24,6 +24,7 @@  block-obj-m = block/
 crypto-obj-y = crypto/
 
 io-obj-y = io/
+io-obj-y += yank.o
 
 endif # CONFIG_SOFTMMU or CONFIG_TOOLS
 
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 777ff5954e..0fa7a364f3 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -48,6 +48,7 @@  struct QIOChannelSocket {
     socklen_t localAddrLen;
     struct sockaddr_storage remoteAddr;
     socklen_t remoteAddrLen;
+    bool yank;
 };
 
 
diff --git a/include/io/channel.h b/include/io/channel.h
index d4557f0930..782b618694 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -132,6 +132,8 @@  struct QIOChannelClass {
                         bool enabled);
     void (*io_set_delay)(QIOChannel *ioc,
                          bool enabled);
+    void (*io_set_yank)(QIOChannel *ioc,
+                        bool enabled);
     off_t (*io_seek)(QIOChannel *ioc,
                      off_t offset,
                      int whence,
@@ -550,6 +552,16 @@  int qio_channel_shutdown(QIOChannel *ioc,
 void qio_channel_set_delay(QIOChannel *ioc,
                            bool enabled);
 
+/**
+ * qio_channel_set_yank:
+ * @ioc: the channel object
+ * @enabled: the new flag state
+ *
+ * Controls wether this channel participates in yanking.
+ */
+void qio_channel_set_yank(QIOChannel *ioc,
+                          bool enabled);
+
 /**
  * qio_channel_set_cork:
  * @ioc: the channel object
diff --git a/io/channel-socket.c b/io/channel-socket.c
index b74f5b92a0..be03946d29 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -26,6 +26,7 @@ 
 #include "io/channel-watch.h"
 #include "trace.h"
 #include "qapi/clone-visitor.h"
+#include "yank.h"
 
 #define SOCKET_MAX_FDS 16
 
@@ -55,6 +56,7 @@  qio_channel_socket_new(void)
 
     sioc = QIO_CHANNEL_SOCKET(object_new(TYPE_QIO_CHANNEL_SOCKET));
     sioc->fd = -1;
+    sioc->yank = 0;
 
     ioc = QIO_CHANNEL(sioc);
     qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
@@ -395,10 +397,19 @@  qio_channel_socket_accept(QIOChannelSocket *ioc,
     return NULL;
 }
 
+static void qio_channel_socket_yank(void *opaque)
+{
+    QIOChannel *ioc = opaque;
+    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
+
+    shutdown(sioc->fd, SHUT_RDWR);
+}
+
 static void qio_channel_socket_init(Object *obj)
 {
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
     ioc->fd = -1;
+    ioc->yank = 0;
 }
 
 static void qio_channel_socket_finalize(Object *obj)
@@ -422,6 +433,9 @@  static void qio_channel_socket_finalize(Object *obj)
         closesocket(ioc->fd);
         ioc->fd = -1;
     }
+    if (ioc->yank) {
+        yank_unregister_function(qio_channel_socket_yank, ioc);
+    }
 }
 
 
@@ -686,6 +700,20 @@  qio_channel_socket_set_delay(QIOChannel *ioc,
                     &v, sizeof(v));
 }
 
+static void
+qio_channel_socket_set_yank(QIOChannel *ioc,
+                            bool enabled)
+{
+    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
+
+    if (sioc->yank) {
+        yank_unregister_function(qio_channel_socket_yank, ioc);
+    }
+    sioc->yank = enabled;
+    if (sioc->yank) {
+        yank_register_function(qio_channel_socket_yank, ioc);
+    }
+}
 
 static void
 qio_channel_socket_set_cork(QIOChannel *ioc,
@@ -784,6 +812,7 @@  static void qio_channel_socket_class_init(ObjectClass *klass,
     ioc_klass->io_shutdown = qio_channel_socket_shutdown;
     ioc_klass->io_set_cork = qio_channel_socket_set_cork;
     ioc_klass->io_set_delay = qio_channel_socket_set_delay;
+    ioc_klass->io_set_yank = qio_channel_socket_set_yank;
     ioc_klass->io_create_watch = qio_channel_socket_create_watch;
     ioc_klass->io_set_aio_fd_handler = qio_channel_socket_set_aio_fd_handler;
 }
diff --git a/io/channel.c b/io/channel.c
index e4376eb0bc..0c4095e0e0 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -373,6 +373,15 @@  void qio_channel_set_delay(QIOChannel *ioc,
     }
 }
 
+void qio_channel_set_yank(QIOChannel *ioc,
+                          bool enabled)
+{
+    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
+
+    if (klass->io_set_yank) {
+        klass->io_set_yank(ioc, enabled);
+    }
+}
 
 void qio_channel_set_cork(QIOChannel *ioc,
                           bool enabled)