* [PATCH 0/2] yank: Always link full yank code
@ 2021-03-23 17:52 Lukas Straub
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Lukas Straub @ 2021-03-23 17:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, Markus Armbruster
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] yank: Remove dependency on qiochannel
2021-03-23 17:52 [PATCH 0/2] yank: Always link full yank code Lukas Straub
@ 2021-03-23 17:52 ` Lukas Straub
2021-03-23 18:03 ` Thomas Huth
2021-03-25 20:29 ` Marc-André Lureau
2021-03-23 17:52 ` [PATCH 2/2] yank: Always link full yank code Lukas Straub
2021-03-23 19:09 ` [PATCH 0/2] " Daniel P. Berrangé
2 siblings, 2 replies; 11+ messages in thread
From: Lukas Straub @ 2021-03-23 17:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, Markus Armbruster
[-- 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 --]
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] yank: Always link full yank code
2021-03-23 17:52 [PATCH 0/2] yank: Always link full yank code Lukas Straub
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
@ 2021-03-23 17:52 ` Lukas Straub
2021-03-23 18:04 ` Thomas Huth
2021-03-25 20:30 ` Marc-André Lureau
2021-03-23 19:09 ` [PATCH 0/2] " Daniel P. Berrangé
2 siblings, 2 replies; 11+ messages in thread
From: Lukas Straub @ 2021-03-23 17:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, Markus Armbruster
[-- 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 --]
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] yank: Remove dependency on qiochannel
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
@ 2021-03-23 18:03 ` Thomas Huth
2021-03-25 20:29 ` Marc-André Lureau
1 sibling, 0 replies; 11+ messages in thread
From: Thomas Huth @ 2021-03-23 18:03 UTC (permalink / raw)
To: Lukas Straub, qemu-devel; +Cc: Paolo Bonzini, Alex Bennee, Markus Armbruster
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] yank: Always link full yank code
2021-03-23 17:52 ` [PATCH 2/2] yank: Always link full yank code Lukas Straub
@ 2021-03-23 18:04 ` Thomas Huth
2021-03-25 20:30 ` Marc-André Lureau
1 sibling, 0 replies; 11+ messages in thread
From: Thomas Huth @ 2021-03-23 18:04 UTC (permalink / raw)
To: Lukas Straub, qemu-devel; +Cc: Paolo Bonzini, Alex Bennee, Markus Armbruster
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] yank: Always link full yank code
2021-03-23 17:52 [PATCH 0/2] yank: Always link full yank code Lukas Straub
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
2021-03-23 17:52 ` [PATCH 2/2] yank: Always link full yank code Lukas Straub
@ 2021-03-23 19:09 ` Daniel P. Berrangé
2021-03-24 11:22 ` Lukas Straub
2 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2021-03-23 19:09 UTC (permalink / raw)
To: Lukas Straub
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] yank: Always link full yank code
2021-03-23 19:09 ` [PATCH 0/2] " Daniel P. Berrangé
@ 2021-03-24 11:22 ` Lukas Straub
2021-03-24 11:36 ` Daniel P. Berrangé
0 siblings, 1 reply; 11+ messages in thread
From: Lukas Straub @ 2021-03-24 11:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] yank: Always link full yank code
2021-03-24 11:22 ` Lukas Straub
@ 2021-03-24 11:36 ` Daniel P. Berrangé
2021-03-24 12:09 ` Lukas Straub
0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2021-03-24 11:36 UTC (permalink / raw)
To: Lukas Straub
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] yank: Always link full yank code
2021-03-24 11:36 ` Daniel P. Berrangé
@ 2021-03-24 12:09 ` Lukas Straub
0 siblings, 0 replies; 11+ messages in thread
From: Lukas Straub @ 2021-03-24 12:09 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] yank: Remove dependency on qiochannel
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
2021-03-23 18:03 ` Thomas Huth
@ 2021-03-25 20:29 ` Marc-André Lureau
1 sibling, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-03-25 20:29 UTC (permalink / raw)
To: Lukas Straub
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] yank: Always link full yank code
2021-03-23 17:52 ` [PATCH 2/2] yank: Always link full yank code Lukas Straub
2021-03-23 18:04 ` Thomas Huth
@ 2021-03-25 20:30 ` Marc-André Lureau
1 sibling, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-03-25 20:30 UTC (permalink / raw)
To: Lukas Straub
Cc: Paolo Bonzini, Thomas Huth, Alex Bennee, qemu-devel, Markus Armbruster
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-25 20:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 17:52 [PATCH 0/2] yank: Always link full yank code Lukas Straub
2021-03-23 17:52 ` [PATCH 1/2] yank: Remove dependency on qiochannel Lukas Straub
2021-03-23 18:03 ` Thomas Huth
2021-03-25 20:29 ` Marc-André Lureau
2021-03-23 17:52 ` [PATCH 2/2] yank: Always link full yank code Lukas Straub
2021-03-23 18:04 ` Thomas Huth
2021-03-25 20:30 ` Marc-André Lureau
2021-03-23 19:09 ` [PATCH 0/2] " Daniel P. Berrangé
2021-03-24 11:22 ` Lukas Straub
2021-03-24 11:36 ` Daniel P. Berrangé
2021-03-24 12:09 ` Lukas Straub
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).