[PATCH 0/2] yank: Always link full yank code

[PATCH 0/2] yank: Always link full yank code

[Lukas Straub]

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

Hello Everyone,
These patches remove yank's dependency on qiochannel and always link it in.
Please Review.

Regards,
Lukas Straub

Lukas Straub (2):
  yank: Remove dependency on qiochannel
  yank: Always link full yank code

 MAINTAINERS                   |  2 +-
 chardev/char-socket.c         | 21 ++++++++++++++-------
 include/qemu/yank.h           | 10 ----------
 migration/channel.c           |  6 ++++--
 migration/meson.build         |  1 +
 migration/multifd.c           |  3 ++-
 migration/qemu-file-channel.c |  3 ++-
 migration/yank_functions.c    | 20 ++++++++++++++++++++
 migration/yank_functions.h    | 17 +++++++++++++++++
 stubs/meson.build             |  1 -
 stubs/yank.c                  | 29 -----------------------------
 util/meson.build              |  2 +-
 util/yank.c                   |  8 --------
 13 files changed, 62 insertions(+), 61 deletions(-)
 create mode 100644 migration/yank_functions.c
 create mode 100644 migration/yank_functions.h
 delete mode 100644 stubs/yank.c

--
2.30.2

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/2] yank: Remove dependency on qiochannel

[Lukas Straub]

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

Remove dependency on qiochannel by removing yank_generic_iochannel and
letting migration and chardev use their own yank function for
iochannel.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 MAINTAINERS                   |  1 +
 chardev/char-socket.c         | 21 ++++++++++++++-------
 include/qemu/yank.h           | 10 ----------
 migration/channel.c           |  6 ++++--
 migration/meson.build         |  1 +
 migration/multifd.c           |  3 ++-
 migration/qemu-file-channel.c |  3 ++-
 migration/yank_functions.c    | 20 ++++++++++++++++++++
 migration/yank_functions.h    | 17 +++++++++++++++++
 stubs/yank.c                  |  6 ------
 util/yank.c                   |  8 --------
 11 files changed, 61 insertions(+), 35 deletions(-)
 create mode 100644 migration/yank_functions.c
 create mode 100644 migration/yank_functions.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9147e9a429..455775c4a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
 S: Odd fixes
 F: util/yank.c
 F: stubs/yank.c
+F: migration/yank_functions*
 F: include/qemu/yank.h
 F: qapi/yank.json

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index f618bdec28..1d455ecca4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -402,6 +402,13 @@ static void remove_hup_source(SocketChardev *s)
     }
 }

+static void char_socket_yank_iochannel(void *opaque)
+{
+    QIOChannel *ioc = QIO_CHANNEL(opaque);
+
+    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
 static void tcp_chr_free_connection(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -424,7 +431,7 @@ static void tcp_chr_free_connection(Chardev *chr)
         (s->state == TCP_CHARDEV_STATE_CONNECTING
         || s->state == TCP_CHARDEV_STATE_CONNECTED)) {
         yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
-                                 yank_generic_iochannel,
+                                 char_socket_yank_iochannel,
                                  QIO_CHANNEL(s->sioc));
     }
     object_unref(OBJECT(s->sioc));
@@ -946,7 +953,7 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
     tcp_chr_set_client_ioc_name(chr, sioc);
     if (s->registered_yank) {
         yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
-                               yank_generic_iochannel,
+                               char_socket_yank_iochannel,
                                QIO_CHANNEL(sioc));
     }
     ret = tcp_chr_new_client(chr, sioc);
@@ -965,7 +972,7 @@ static void tcp_chr_accept(QIONetListener *listener,
     tcp_chr_set_client_ioc_name(chr, cioc);
     if (s->registered_yank) {
         yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
-                               yank_generic_iochannel,
+                               char_socket_yank_iochannel,
                                QIO_CHANNEL(cioc));
     }
     tcp_chr_new_client(chr, cioc);
@@ -985,7 +992,7 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
     }
     if (s->registered_yank) {
         yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
-                               yank_generic_iochannel,
+                               char_socket_yank_iochannel,
                                QIO_CHANNEL(sioc));
     }
     tcp_chr_new_client(chr, sioc);
@@ -1005,7 +1012,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
     tcp_chr_set_client_ioc_name(chr, sioc);
     if (s->registered_yank) {
         yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
-                               yank_generic_iochannel,
+                               char_socket_yank_iochannel,
                                QIO_CHANNEL(sioc));
     }
     tcp_chr_new_client(chr, sioc);
@@ -1138,7 +1145,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
         tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
         if (s->registered_yank) {
             yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
-                                     yank_generic_iochannel,
+                                     char_socket_yank_iochannel,
                                      QIO_CHANNEL(sioc));
         }
         check_report_connect_error(chr, err);
@@ -1176,7 +1183,7 @@ static void tcp_chr_connect_client_async(Chardev *chr)
     tcp_chr_set_client_ioc_name(chr, sioc);
     if (s->registered_yank) {
         yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
-                               yank_generic_iochannel,
+                               char_socket_yank_iochannel,
                                QIO_CHANNEL(sioc));
     }
     /*
diff --git a/include/qemu/yank.h b/include/qemu/yank.h
index 5b93c70cbf..5375a1f195 100644
--- a/include/qemu/yank.h
+++ b/include/qemu/yank.h
@@ -73,16 +73,6 @@ void yank_unregister_function(const YankInstance *instance,
                               YankFn *func,
                               void *opaque);

-/**
- * yank_generic_iochannel: Generic yank function for iochannel
- *
- * This is a generic yank function which will call qio_channel_shutdown on the
- * provided QIOChannel.
- *
- * @opaque: QIOChannel to shutdown
- */
-void yank_generic_iochannel(void *opaque);
-
 #define BLOCKDEV_YANK_INSTANCE(the_node_name) (&(YankInstance) { \
         .type = YANK_INSTANCE_TYPE_BLOCK_NODE, \
         .u.block_node.node_name = (the_node_name) })
diff --git a/migration/channel.c b/migration/channel.c
index 35fe234e9c..c9ee902021 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -20,6 +20,7 @@
 #include "io/channel-tls.h"
 #include "io/channel-socket.h"
 #include "qemu/yank.h"
+#include "yank_functions.h"

 /**
  * @migration_channel_process_incoming - Create new incoming migration channel
@@ -38,7 +39,8 @@ void migration_channel_process_incoming(QIOChannel *ioc)
         ioc, object_get_typename(OBJECT(ioc)));

     if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
-        yank_register_function(MIGRATION_YANK_INSTANCE, yank_generic_iochannel,
+        yank_register_function(MIGRATION_YANK_INSTANCE,
+                               migration_yank_iochannel,
                                QIO_CHANNEL(ioc));
     }

@@ -76,7 +78,7 @@ void migration_channel_connect(MigrationState *s,
     if (!error) {
         if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
             yank_register_function(MIGRATION_YANK_INSTANCE,
-                                   yank_generic_iochannel,
+                                   migration_yank_iochannel,
                                    QIO_CHANNEL(ioc));
         }

diff --git a/migration/meson.build b/migration/meson.build
index 9645f44005..2cfa8eed72 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -6,6 +6,7 @@ migration_files = files(
   'vmstate.c',
   'qemu-file-channel.c',
   'qemu-file.c',
+  'yank_functions.c',
 )
 softmmu_ss.add(migration_files)

diff --git a/migration/multifd.c b/migration/multifd.c
index 03527c564c..a6677c45c8 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -27,6 +27,7 @@

 #include "qemu/yank.h"
 #include "io/channel-socket.h"
+#include "yank_functions.h"

 /* Multiple fd's */

@@ -989,7 +990,7 @@ int multifd_load_cleanup(Error **errp)
         if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
             && OBJECT(p->c)->ref == 1) {
             yank_unregister_function(MIGRATION_YANK_INSTANCE,
-                                     yank_generic_iochannel,
+                                     migration_yank_iochannel,
                                      QIO_CHANNEL(p->c));
         }

diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index afc3a7f642..876d05a540 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -28,6 +28,7 @@
 #include "io/channel-socket.h"
 #include "qemu/iov.h"
 #include "qemu/yank.h"
+#include "yank_functions.h"


 static ssize_t channel_writev_buffer(void *opaque,
@@ -108,7 +109,7 @@ static int channel_close(void *opaque, Error **errp)
     if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
         && OBJECT(ioc)->ref == 1) {
         yank_unregister_function(MIGRATION_YANK_INSTANCE,
-                                 yank_generic_iochannel,
+                                 migration_yank_iochannel,
                                  QIO_CHANNEL(ioc));
     }
     object_unref(OBJECT(ioc));
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
new file mode 100644
index 0000000000..96c90e17dc
--- /dev/null
+++ b/migration/yank_functions.c
@@ -0,0 +1,20 @@
+/*
+ * migration yank functions
+ *
+ * Copyright (c) Lukas Straub <lukasstraub2@web.de>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "io/channel.h"
+#include "yank_functions.h"
+
+void migration_yank_iochannel(void *opaque)
+{
+    QIOChannel *ioc = QIO_CHANNEL(opaque);
+
+    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
new file mode 100644
index 0000000000..055ea22523
--- /dev/null
+++ b/migration/yank_functions.h
@@ -0,0 +1,17 @@
+/*
+ * migration yank functions
+ *
+ * Copyright (c) Lukas Straub <lukasstraub2@web.de>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+/**
+ * migration_yank_iochannel: yank function for iochannel
+ *
+ * This yank function will call qio_channel_shutdown on the provided QIOChannel.
+ *
+ * @opaque: QIOChannel to shutdown
+ */
+void migration_yank_iochannel(void *opaque);
diff --git a/stubs/yank.c b/stubs/yank.c
index 6090416065..11b24fc057 100644
--- a/stubs/yank.c
+++ b/stubs/yank.c
@@ -21,9 +21,3 @@ void yank_unregister_function(const YankInstance *instance,
                               void *opaque)
 {
 }
-
-void yank_generic_iochannel(void *opaque)
-{
-}
-
-
diff --git a/util/yank.c b/util/yank.c
index fc08f65209..abf47c346d 100644
--- a/util/yank.c
+++ b/util/yank.c
@@ -15,7 +15,6 @@
 #include "qapi/qapi-commands-yank.h"
 #include "qapi/qapi-visit-yank.h"
 #include "qapi/clone-visitor.h"
-#include "io/channel.h"
 #include "qemu/yank.h"

 struct YankFuncAndParam {
@@ -151,13 +150,6 @@ void yank_unregister_function(const YankInstance *instance,
     abort();
 }

-void yank_generic_iochannel(void *opaque)
-{
-    QIOChannel *ioc = QIO_CHANNEL(opaque);
-
-    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
-}
-
 void qmp_yank(YankInstanceList *instances,
               Error **errp)
 {
--
2.30.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] yank: Always link full yank code

[Lukas Straub]

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

Yank now only depends on util and can be always linked in. Also remove
the stubs as they are not needed anymore.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 MAINTAINERS       |  1 -
 stubs/meson.build |  1 -
 stubs/yank.c      | 23 -----------------------
 util/meson.build  |  2 +-
 4 files changed, 1 insertion(+), 26 deletions(-)
 delete mode 100644 stubs/yank.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 455775c4a3..77259c031d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2820,7 +2820,6 @@ Yank feature
 M: Lukas Straub <lukasstraub2@web.de>
 S: Odd fixes
 F: util/yank.c
-F: stubs/yank.c
 F: migration/yank_functions*
 F: include/qemu/yank.h
 F: qapi/yank.json
diff --git a/stubs/meson.build b/stubs/meson.build
index 8a3e804cf0..be6f6d609e 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -46,7 +46,6 @@ stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
 stub_ss.add(files('cpu-synchronize-state.c'))
 if have_block
-  stub_ss.add(files('yank.c'))
   stub_ss.add(files('replay-tools.c'))
 endif
 if have_system
diff --git a/stubs/yank.c b/stubs/yank.c
deleted file mode 100644
index 11b24fc057..0000000000
--- a/stubs/yank.c
+++ /dev/null
@@ -1,23 +0,0 @@
-#include "qemu/osdep.h"
-#include "qemu/yank.h"
-
-bool yank_register_instance(const YankInstance *instance, Error **errp)
-{
-    return true;
-}
-
-void yank_unregister_instance(const YankInstance *instance)
-{
-}
-
-void yank_register_function(const YankInstance *instance,
-                            YankFn *func,
-                            void *opaque)
-{
-}
-
-void yank_unregister_function(const YankInstance *instance,
-                              YankFn *func,
-                              void *opaque)
-{
-}
diff --git a/util/meson.build b/util/meson.build
index 984fba965f..510765cde4 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -43,6 +43,7 @@ util_ss.add(files('stats64.c'))
 util_ss.add(files('systemd.c'))
 util_ss.add(when: 'CONFIG_POSIX', if_true: files('drm.c'))
 util_ss.add(files('guest-random.c'))
+util_ss.add(files('yank.c'))

 if have_user
   util_ss.add(files('selfmap.c'))
@@ -51,7 +52,6 @@ endif
 if have_system
   util_ss.add(files('crc-ccitt.c'))
   util_ss.add(when: 'CONFIG_GIO', if_true: [files('dbus.c'), gio])
-  util_ss.add(files('yank.c'))
   util_ss.add(when: 'CONFIG_LINUX', if_true: files('userfaultfd.c'))
 endif

--
2.30.2

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] yank: Remove dependency on qiochannel

[Thomas Huth]

On 23/03/2021 18.52, Lukas Straub wrote:
> Remove dependency on qiochannel by removing yank_generic_iochannel and
> letting migration and chardev use their own yank function for
> iochannel.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   MAINTAINERS                   |  1 +
>   chardev/char-socket.c         | 21 ++++++++++++++-------
>   include/qemu/yank.h           | 10 ----------
>   migration/channel.c           |  6 ++++--
>   migration/meson.build         |  1 +
>   migration/multifd.c           |  3 ++-
>   migration/qemu-file-channel.c |  3 ++-
>   migration/yank_functions.c    | 20 ++++++++++++++++++++
>   migration/yank_functions.h    | 17 +++++++++++++++++
>   stubs/yank.c                  |  6 ------
>   util/yank.c                   |  8 --------
>   11 files changed, 61 insertions(+), 35 deletions(-)
>   create mode 100644 migration/yank_functions.c
>   create mode 100644 migration/yank_functions.h

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] yank: Always link full yank code

[Thomas Huth]

On 23/03/2021 18.52, Lukas Straub wrote:
> Yank now only depends on util and can be always linked in. Also remove
> the stubs as they are not needed anymore.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   MAINTAINERS       |  1 -
>   stubs/meson.build |  1 -
>   stubs/yank.c      | 23 -----------------------
>   util/meson.build  |  2 +-
>   4 files changed, 1 insertion(+), 26 deletions(-)
>   delete mode 100644 stubs/yank.c

This indeed looks like the best solution to me. Thanks!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/2] yank: Always link full yank code

[Daniel P. Berrangé]

On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote:
> Hello Everyone,
> These patches remove yank's dependency on qiochannel and always link it in.
> Please Review.

It would be useful if the cover letter or commit messages explained
to potential reviewers why this is being changed... The first patch
feels like a regression to me, without seeing an explanation why
it is desirable.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] yank: Always link full yank code

[Lukas Straub]

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

On Tue, 23 Mar 2021 19:09:15 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote:
> > Hello Everyone,
> > These patches remove yank's dependency on qiochannel and always link it in.
> > Please Review.  
> 
> It would be useful if the cover letter or commit messages explained
> to potential reviewers why this is being changed... The first patch
> feels like a regression to me, without seeing an explanation why
> it is desirable.

Yes, sorry. There are two reasons for this patchset:
-To exercise the full yank code (with checks for registering and unregistering
 of yank functions and instances) in existing tests and in the new yank test
 (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/)
-To replace "[PATCH] yank: Avoid linking into executables that don't want it"
 (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/)
 Now we always link in yank, but without pulling in other dependencies.

Regards,
Lukas Straub

> 
> Regards,
> Daniel



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] yank: Always link full yank code

[Daniel P. Berrangé]

On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote:
> On Tue, 23 Mar 2021 19:09:15 +0000
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote:
> > > Hello Everyone,
> > > These patches remove yank's dependency on qiochannel and always link it in.
> > > Please Review.  
> > 
> > It would be useful if the cover letter or commit messages explained
> > to potential reviewers why this is being changed... The first patch
> > feels like a regression to me, without seeing an explanation why
> > it is desirable.
> 
> Yes, sorry. There are two reasons for this patchset:
> -To exercise the full yank code (with checks for registering and unregistering
>  of yank functions and instances) in existing tests and in the new yank test
>  (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/)
> -To replace "[PATCH] yank: Avoid linking into executables that don't want it"
>  (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/)
>  Now we always link in yank, but without pulling in other dependencies.

Conceptually, the real world usage of yank is in combination with parts
of QEMU doing I/O, and so they will always have the "io" subsystem
available. Thus it feels wrong to be refactoring this to avoid an
"io" dependancy.  I think this probably suggests that the yank code
shouldn't be in libqemuutil.la, but instead part of the "io" subsystem
to make the association/dependency explicit


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] yank: Always link full yank code

[Lukas Straub]

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

On Wed, 24 Mar 2021 11:36:13 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote:
> > On Tue, 23 Mar 2021 19:09:15 +0000
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote:  
> > > > Hello Everyone,
> > > > These patches remove yank's dependency on qiochannel and always link it in.
> > > > Please Review.    
> > > 
> > > It would be useful if the cover letter or commit messages explained
> > > to potential reviewers why this is being changed... The first patch
> > > feels like a regression to me, without seeing an explanation why
> > > it is desirable.  
> > 
> > Yes, sorry. There are two reasons for this patchset:
> > -To exercise the full yank code (with checks for registering and unregistering
> >  of yank functions and instances) in existing tests and in the new yank test
> >  (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstraub2@web.de/)
> > -To replace "[PATCH] yank: Avoid linking into executables that don't want it"
> >  (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-armbru@redhat.com/)
> >  Now we always link in yank, but without pulling in other dependencies.  
> 
> Conceptually, the real world usage of yank is in combination with parts
> of QEMU doing I/O, and so they will always have the "io" subsystem
> available. Thus it feels wrong to be refactoring this to avoid an
> "io" dependancy.  I think this probably suggests that the yank code
> shouldn't be in libqemuutil.la, but instead part of the "io" subsystem
> to make the association/dependency explicit

Yes, makes sense. On the other hand I think that the yank functions should be
separate from the yank core anyway.

Regards,
Lukas Straub

> 
> Regards,
> Daniel



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] yank: Remove dependency on qiochannel

[Marc-André Lureau]

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

On Tue, Mar 23, 2021 at 9:54 PM Lukas Straub <lukasstraub2@web.de> wrote:

> Remove dependency on qiochannel by removing yank_generic_iochannel and
> letting migration and chardev use their own yank function for
> iochannel.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  MAINTAINERS                   |  1 +
>  chardev/char-socket.c         | 21 ++++++++++++++-------
>  include/qemu/yank.h           | 10 ----------
>  migration/channel.c           |  6 ++++--
>  migration/meson.build         |  1 +
>  migration/multifd.c           |  3 ++-
>  migration/qemu-file-channel.c |  3 ++-
>  migration/yank_functions.c    | 20 ++++++++++++++++++++
>  migration/yank_functions.h    | 17 +++++++++++++++++
>  stubs/yank.c                  |  6 ------
>  util/yank.c                   |  8 --------
>  11 files changed, 61 insertions(+), 35 deletions(-)
>  create mode 100644 migration/yank_functions.c
>  create mode 100644 migration/yank_functions.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9147e9a429..455775c4a3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
>  S: Odd fixes
>  F: util/yank.c
>  F: stubs/yank.c
> +F: migration/yank_functions*
>  F: include/qemu/yank.h
>  F: qapi/yank.json
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index f618bdec28..1d455ecca4 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -402,6 +402,13 @@ static void remove_hup_source(SocketChardev *s)
>      }
>  }
>
> +static void char_socket_yank_iochannel(void *opaque)
> +{
> +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> +
> +    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +
>  static void tcp_chr_free_connection(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -424,7 +431,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>          (s->state == TCP_CHARDEV_STATE_CONNECTING
>          || s->state == TCP_CHARDEV_STATE_CONNECTED)) {
>          yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                                 yank_generic_iochannel,
> +                                 char_socket_yank_iochannel,
>                                   QIO_CHANNEL(s->sioc));
>      }
>      object_unref(OBJECT(s->sioc));
> @@ -946,7 +953,7 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      if (s->registered_yank) {
>          yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                               yank_generic_iochannel,
> +                               char_socket_yank_iochannel,
>                                 QIO_CHANNEL(sioc));
>      }
>      ret = tcp_chr_new_client(chr, sioc);
> @@ -965,7 +972,7 @@ static void tcp_chr_accept(QIONetListener *listener,
>      tcp_chr_set_client_ioc_name(chr, cioc);
>      if (s->registered_yank) {
>          yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                               yank_generic_iochannel,
> +                               char_socket_yank_iochannel,
>                                 QIO_CHANNEL(cioc));
>      }
>      tcp_chr_new_client(chr, cioc);
> @@ -985,7 +992,7 @@ static int tcp_chr_connect_client_sync(Chardev *chr,
> Error **errp)
>      }
>      if (s->registered_yank) {
>          yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                               yank_generic_iochannel,
> +                               char_socket_yank_iochannel,
>                                 QIO_CHANNEL(sioc));
>      }
>      tcp_chr_new_client(chr, sioc);
> @@ -1005,7 +1012,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      if (s->registered_yank) {
>          yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                               yank_generic_iochannel,
> +                               char_socket_yank_iochannel,
>                                 QIO_CHANNEL(sioc));
>      }
>      tcp_chr_new_client(chr, sioc);
> @@ -1138,7 +1145,7 @@ static void qemu_chr_socket_connected(QIOTask *task,
> void *opaque)
>          tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
>          if (s->registered_yank) {
>              yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                                     yank_generic_iochannel,
> +                                     char_socket_yank_iochannel,
>                                       QIO_CHANNEL(sioc));
>          }
>          check_report_connect_error(chr, err);
> @@ -1176,7 +1183,7 @@ static void tcp_chr_connect_client_async(Chardev
> *chr)
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      if (s->registered_yank) {
>          yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> -                               yank_generic_iochannel,
> +                               char_socket_yank_iochannel,
>                                 QIO_CHANNEL(sioc));
>      }
>      /*
> diff --git a/include/qemu/yank.h b/include/qemu/yank.h
> index 5b93c70cbf..5375a1f195 100644
> --- a/include/qemu/yank.h
> +++ b/include/qemu/yank.h
> @@ -73,16 +73,6 @@ void yank_unregister_function(const YankInstance
> *instance,
>                                YankFn *func,
>                                void *opaque);
>
> -/**
> - * yank_generic_iochannel: Generic yank function for iochannel
> - *
> - * This is a generic yank function which will call qio_channel_shutdown
> on the
> - * provided QIOChannel.
> - *
> - * @opaque: QIOChannel to shutdown
> - */
> -void yank_generic_iochannel(void *opaque);
> -
>  #define BLOCKDEV_YANK_INSTANCE(the_node_name) (&(YankInstance) { \
>          .type = YANK_INSTANCE_TYPE_BLOCK_NODE, \
>          .u.block_node.node_name = (the_node_name) })
> diff --git a/migration/channel.c b/migration/channel.c
> index 35fe234e9c..c9ee902021 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -20,6 +20,7 @@
>  #include "io/channel-tls.h"
>  #include "io/channel-socket.h"
>  #include "qemu/yank.h"
> +#include "yank_functions.h"
>
>  /**
>   * @migration_channel_process_incoming - Create new incoming migration
> channel
> @@ -38,7 +39,8 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>          ioc, object_get_typename(OBJECT(ioc)));
>
>      if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -        yank_register_function(MIGRATION_YANK_INSTANCE,
> yank_generic_iochannel,
> +        yank_register_function(MIGRATION_YANK_INSTANCE,
> +                               migration_yank_iochannel,
>                                 QIO_CHANNEL(ioc));
>      }
>
> @@ -76,7 +78,7 @@ void migration_channel_connect(MigrationState *s,
>      if (!error) {
>          if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
>              yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                   yank_generic_iochannel,
> +                                   migration_yank_iochannel,
>                                     QIO_CHANNEL(ioc));
>          }
>
> diff --git a/migration/meson.build b/migration/meson.build
> index 9645f44005..2cfa8eed72 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -6,6 +6,7 @@ migration_files = files(
>    'vmstate.c',
>    'qemu-file-channel.c',
>    'qemu-file.c',
> +  'yank_functions.c',
>  )
>  softmmu_ss.add(migration_files)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 03527c564c..a6677c45c8 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -27,6 +27,7 @@
>
>  #include "qemu/yank.h"
>  #include "io/channel-socket.h"
> +#include "yank_functions.h"
>
>  /* Multiple fd's */
>
> @@ -989,7 +990,7 @@ int multifd_load_cleanup(Error **errp)
>          if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
>              && OBJECT(p->c)->ref == 1) {
>              yank_unregister_function(MIGRATION_YANK_INSTANCE,
> -                                     yank_generic_iochannel,
> +                                     migration_yank_iochannel,
>                                       QIO_CHANNEL(p->c));
>          }
>
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index afc3a7f642..876d05a540 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -28,6 +28,7 @@
>  #include "io/channel-socket.h"
>  #include "qemu/iov.h"
>  #include "qemu/yank.h"
> +#include "yank_functions.h"
>
>
>  static ssize_t channel_writev_buffer(void *opaque,
> @@ -108,7 +109,7 @@ static int channel_close(void *opaque, Error **errp)
>      if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
>          && OBJECT(ioc)->ref == 1) {
>          yank_unregister_function(MIGRATION_YANK_INSTANCE,
> -                                 yank_generic_iochannel,
> +                                 migration_yank_iochannel,
>                                   QIO_CHANNEL(ioc));
>      }
>      object_unref(OBJECT(ioc));
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> new file mode 100644
> index 0000000000..96c90e17dc
> --- /dev/null
> +++ b/migration/yank_functions.c
> @@ -0,0 +1,20 @@
> +/*
> + * migration yank functions
> + *
> + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "io/channel.h"
> +#include "yank_functions.h"
> +
> +void migration_yank_iochannel(void *opaque)
> +{
> +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> +
> +    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> diff --git a/migration/yank_functions.h b/migration/yank_functions.h
> new file mode 100644
> index 0000000000..055ea22523
> --- /dev/null
> +++ b/migration/yank_functions.h
> @@ -0,0 +1,17 @@
> +/*
> + * migration yank functions
> + *
> + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +/**
> + * migration_yank_iochannel: yank function for iochannel
> + *
> + * This yank function will call qio_channel_shutdown on the provided
> QIOChannel.
> + *
> + * @opaque: QIOChannel to shutdown
> + */
> +void migration_yank_iochannel(void *opaque);
> diff --git a/stubs/yank.c b/stubs/yank.c
> index 6090416065..11b24fc057 100644
> --- a/stubs/yank.c
> +++ b/stubs/yank.c
> @@ -21,9 +21,3 @@ void yank_unregister_function(const YankInstance
> *instance,
>                                void *opaque)
>  {
>  }
> -
> -void yank_generic_iochannel(void *opaque)
> -{
> -}
> -
> -
> diff --git a/util/yank.c b/util/yank.c
> index fc08f65209..abf47c346d 100644
> --- a/util/yank.c
> +++ b/util/yank.c
> @@ -15,7 +15,6 @@
>  #include "qapi/qapi-commands-yank.h"
>  #include "qapi/qapi-visit-yank.h"
>  #include "qapi/clone-visitor.h"
> -#include "io/channel.h"
>  #include "qemu/yank.h"
>
>  struct YankFuncAndParam {
> @@ -151,13 +150,6 @@ void yank_unregister_function(const YankInstance
> *instance,
>      abort();
>  }
>
> -void yank_generic_iochannel(void *opaque)
> -{
> -    QIOChannel *ioc = QIO_CHANNEL(opaque);
> -
> -    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> -}
> -
>  void qmp_yank(YankInstanceList *instances,
>                Error **errp)
>  {
> --
> 2.30.2
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 14682 bytes --]

Re: [PATCH 2/2] yank: Always link full yank code

[Marc-André Lureau]

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

On Tue, Mar 23, 2021 at 9:57 PM Lukas Straub <lukasstraub2@web.de> wrote:

> Yank now only depends on util and can be always linked in. Also remove
> the stubs as they are not needed anymore.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  MAINTAINERS       |  1 -
>  stubs/meson.build |  1 -
>  stubs/yank.c      | 23 -----------------------
>  util/meson.build  |  2 +-
>  4 files changed, 1 insertion(+), 26 deletions(-)
>  delete mode 100644 stubs/yank.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 455775c4a3..77259c031d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2820,7 +2820,6 @@ Yank feature
>  M: Lukas Straub <lukasstraub2@web.de>
>  S: Odd fixes
>  F: util/yank.c
> -F: stubs/yank.c
>  F: migration/yank_functions*
>  F: include/qemu/yank.h
>  F: qapi/yank.json
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 8a3e804cf0..be6f6d609e 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -46,7 +46,6 @@ stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
>  stub_ss.add(files('cpu-synchronize-state.c'))
>  if have_block
> -  stub_ss.add(files('yank.c'))
>    stub_ss.add(files('replay-tools.c'))
>  endif
>  if have_system
> diff --git a/stubs/yank.c b/stubs/yank.c
> deleted file mode 100644
> index 11b24fc057..0000000000
> --- a/stubs/yank.c
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -#include "qemu/osdep.h"
> -#include "qemu/yank.h"
> -
> -bool yank_register_instance(const YankInstance *instance, Error **errp)
> -{
> -    return true;
> -}
> -
> -void yank_unregister_instance(const YankInstance *instance)
> -{
> -}
> -
> -void yank_register_function(const YankInstance *instance,
> -                            YankFn *func,
> -                            void *opaque)
> -{
> -}
> -
> -void yank_unregister_function(const YankInstance *instance,
> -                              YankFn *func,
> -                              void *opaque)
> -{
> -}
> diff --git a/util/meson.build b/util/meson.build
> index 984fba965f..510765cde4 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -43,6 +43,7 @@ util_ss.add(files('stats64.c'))
>  util_ss.add(files('systemd.c'))
>  util_ss.add(when: 'CONFIG_POSIX', if_true: files('drm.c'))
>  util_ss.add(files('guest-random.c'))
> +util_ss.add(files('yank.c'))
>
>  if have_user
>    util_ss.add(files('selfmap.c'))
> @@ -51,7 +52,6 @@ endif
>  if have_system
>    util_ss.add(files('crc-ccitt.c'))
>    util_ss.add(when: 'CONFIG_GIO', if_true: [files('dbus.c'), gio])
> -  util_ss.add(files('yank.c'))
>    util_ss.add(when: 'CONFIG_LINUX', if_true: files('userfaultfd.c'))
>  endif
>
> --
> 2.30.2
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4001 bytes --]