Re: [PATCH v4 1/4] Introduce yank feature
On Fri, 19 Jun 2020 17:52:40 +0100 Daniel P. Berrangé wrote: > On Fri, Jun 19, 2020 at 06:29:24PM +0200, Lukas Straub wrote: > > On Wed, 17 Jun 2020 16:12:40 +0100 > > Stefan Hajnoczi wrote: > > > > > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote: > > > > +static struct YankInstance *yank_find_instance(char *name) > > > > > > There are const char * -> char * casts in later patches. Please use > > > const char * where possible. Callers shouldn't need to cast away const. > > > > nbd and chardev generate the instance name dynamically so it > > needs to be char *, but in migration it's hardcoded. > > I think you're looking at it from the wrong perspective. > > The yank_find_instance() method never modifies the 'name' paramater > that it receives. Therefore it should be "const char *". Likewise > for the other yank_*() methods in fact. > > The caller can have a char *, or a const char * as suits its needs. > Either can be passed into the yank_* methods and will gain const-ness > from the POV of yank code. Makes sense, I will change it in the next version. Thanks, Lukas Straub > > Regards, > Daniel pgphQiooTBERI.pgp Description: OpenPGP digital signature
Re: [PATCH v4 1/4] Introduce yank feature
On Fri, Jun 19, 2020 at 04:23:50PM +0200, Lukas Straub wrote:
> On Tue, 16 Jun 2020 15:39:57 +0100
> Daniel P. Berrangé wrote:
>
> > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > > The yank feature allows to recover from hanging qemu by "yanking"
> > > at various parts. Other qemu systems can register themselves and
> > > multiple yank functions. Then all yank functions for selected
> > > instances can be called by the 'yank' out-of-band qmp command.
> > > Available instances can be queried by a 'query-yank' oob command.
> > >
> > > Signed-off-by: Lukas Straub
> > > ---
> > > qapi/misc.json | 45 +
> > > yank.c | 174 +
> > > yank.h | 67 +++
> > > 3 files changed, 286 insertions(+)
> > > create mode 100644 yank.c
> > > create mode 100644 yank.h
> >
> > > +void yank_register_function(char *instance_name, YankFn *func, void
> > > *opaque)
> > > +{
> > > +struct YankInstance *instance;
> > > +struct YankFuncAndParam *entry;
> > > +
> > > +qemu_mutex_lock(&lock);
> > > +instance = yank_find_instance(instance_name);
> > > +assert(instance);
> > > +
> > > +entry = g_slice_new(struct YankFuncAndParam);
> > > +entry->func = func;
> > > +entry->opaque = opaque;
> > > +
> > > +QLIST_INSERT_HEAD(&instance->yankfns, entry, next);
> > > +qemu_mutex_unlock(&lock);
> > > +}
> > > +
> > > +void yank_unregister_function(char *instance_name, YankFn *func, void
> > > *opaque)
> > > +{
> > > +struct YankInstance *instance;
> > > +struct YankFuncAndParam *entry;
> > > +
> > > +qemu_mutex_lock(&lock);
> > > +instance = yank_find_instance(instance_name);
> > > +assert(instance);
> > > +
> > > +QLIST_FOREACH(entry, &instance->yankfns, next) {
> > > +if (entry->func == func && entry->opaque == opaque) {
> > > +QLIST_REMOVE(entry, next);
> > > +g_slice_free(struct YankFuncAndParam, entry);
> > > +qemu_mutex_unlock(&lock);
> > > +return;
> > > +}
> > > +}
> > > +
> > > +abort();
> > > +}
> >
> > Since the NBD impl no longer needs to register multiple different functions
> > on the same insance_nane, these methods could be be simplified, to only
> > accept a single function, instead of keeping a whole list. This would avoid
> > need to pass a function into the unregister() method at all.
>
> Multiple yank functions are still needed for multifd migration.
Oh I missed that subtlety, so fine to ignore my suggestion.
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 v4 1/4] Introduce yank feature
On Fri, Jun 19, 2020 at 06:29:24PM +0200, Lukas Straub wrote: > On Wed, 17 Jun 2020 16:12:40 +0100 > Stefan Hajnoczi wrote: > > > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote: > > > +static struct YankInstance *yank_find_instance(char *name) > > > > There are const char * -> char * casts in later patches. Please use > > const char * where possible. Callers shouldn't need to cast away const. > > nbd and chardev generate the instance name dynamically so it > needs to be char *, but in migration it's hardcoded. I think you're looking at it from the wrong perspective. The yank_find_instance() method never modifies the 'name' paramater that it receives. Therefore it should be "const char *". Likewise for the other yank_*() methods in fact. The caller can have a char *, or a const char * as suits its needs. Either can be passed into the yank_* methods and will gain const-ness from the POV of yank code. 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 v4 1/4] Introduce yank feature
On Wed, 17 Jun 2020 16:12:40 +0100 Stefan Hajnoczi wrote: > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote: > > +static struct YankInstance *yank_find_instance(char *name) > > There are const char * -> char * casts in later patches. Please use > const char * where possible. Callers shouldn't need to cast away const. nbd and chardev generate the instance name dynamically so it needs to be char *, but in migration it's hardcoded. Thanks, Lukas Straub pgpLRvpG4Txdl.pgp Description: OpenPGP digital signature
Re: [PATCH v4 1/4] Introduce yank feature
On Tue, 16 Jun 2020 15:39:57 +0100
Daniel P. Berrangé wrote:
> On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > The yank feature allows to recover from hanging qemu by "yanking"
> > at various parts. Other qemu systems can register themselves and
> > multiple yank functions. Then all yank functions for selected
> > instances can be called by the 'yank' out-of-band qmp command.
> > Available instances can be queried by a 'query-yank' oob command.
> >
> > Signed-off-by: Lukas Straub
> > ---
> > qapi/misc.json | 45 +
> > yank.c | 174 +
> > yank.h | 67 +++
> > 3 files changed, 286 insertions(+)
> > create mode 100644 yank.c
> > create mode 100644 yank.h
>
> > +void yank_register_function(char *instance_name, YankFn *func, void
> > *opaque)
> > +{
> > +struct YankInstance *instance;
> > +struct YankFuncAndParam *entry;
> > +
> > +qemu_mutex_lock(&lock);
> > +instance = yank_find_instance(instance_name);
> > +assert(instance);
> > +
> > +entry = g_slice_new(struct YankFuncAndParam);
> > +entry->func = func;
> > +entry->opaque = opaque;
> > +
> > +QLIST_INSERT_HEAD(&instance->yankfns, entry, next);
> > +qemu_mutex_unlock(&lock);
> > +}
> > +
> > +void yank_unregister_function(char *instance_name, YankFn *func, void
> > *opaque)
> > +{
> > +struct YankInstance *instance;
> > +struct YankFuncAndParam *entry;
> > +
> > +qemu_mutex_lock(&lock);
> > +instance = yank_find_instance(instance_name);
> > +assert(instance);
> > +
> > +QLIST_FOREACH(entry, &instance->yankfns, next) {
> > +if (entry->func == func && entry->opaque == opaque) {
> > +QLIST_REMOVE(entry, next);
> > +g_slice_free(struct YankFuncAndParam, entry);
> > +qemu_mutex_unlock(&lock);
> > +return;
> > +}
> > +}
> > +
> > +abort();
> > +}
>
> Since the NBD impl no longer needs to register multiple different functions
> on the same insance_nane, these methods could be be simplified, to only
> accept a single function, instead of keeping a whole list. This would avoid
> need to pass a function into the unregister() method at all.
Multiple yank functions are still needed for multifd migration.
> I don't consider this a blocker though, so you pick whether to do this
> or not.
>
>
> > diff --git a/yank.h b/yank.h
> > new file mode 100644
> > index 00..f1c8743b72
> > --- /dev/null
> > +++ b/yank.h
> > @@ -0,0 +1,67 @@
> > +
>
> Missing license header
I will fix that in the next version.
Thanks,
Lukas Straub
>
> > +#ifndef YANK_H
> > +#define YANK_H
> > +
> > +typedef void (YankFn) (void *opaque);
> > +
> > +/**
> > + * yank_register_instance: Register a new instance.
> > + *
> > + * This registers a new instance for yanking. Must be called before any
> > yank
> > + * function is registered for this instance.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The globally unique name of the instance.
> > + */
> > +void yank_register_instance(char *instance_name);
> > +
> > +/**
> > + * yank_unregister_instance: Unregister a instance.
> > + *
> > + * This unregisters a instance. Must be called only after every yank
> > function
> > + * of the instance has been unregistered.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance.
> > + */
> > +void yank_unregister_instance(char *instance_name);
> > +
> > +/**
> > + * yank_register_function: Register a yank function
> > + *
> > + * This registers a yank function. All limitations of qmp oob commands
> > apply
> > + * to the yank function as well.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: The yank function
> > + * @opaque: Will be passed to the yank function
> > + */
> > +void yank_register_function(char *instance_name, YankFn *func, void
> > *opaque);
> > +
> > +/**
> > + * yank_unregister_function: Unregister a yank function
> > + *
> > + * This unregisters a yank function.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: func that was passed to yank_register_function
> > + * @opaque: opaque that was passed to yank_register_function
> > + */
> > +void yank_unregister_function(char *instance_name, YankFn *func, void
> > *opaque);
> > +
> > +/**
> > + * yank_unregister_function: 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);
> > +#endif
>
>
>
> Regards,
> Daniel
pgp_UVHIYRggx.pgp
Description: OpenPGP digital signature
Re: [PATCH v4 1/4] Introduce yank feature
On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote: > +static struct YankInstance *yank_find_instance(char *name) There are const char * -> char * casts in later patches. Please use const char * where possible. Callers shouldn't need to cast away const. signature.asc Description: PGP signature
Re: [PATCH v4 1/4] Introduce yank feature
On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote: > The yank feature allows to recover from hanging qemu by "yanking" > at various parts. Other qemu systems can register themselves and > multiple yank functions. Then all yank functions for selected > instances can be called by the 'yank' out-of-band qmp command. > Available instances can be queried by a 'query-yank' oob command. > > Signed-off-by: Lukas Straub > --- > qapi/misc.json | 45 + > yank.c | 174 + > yank.h | 67 +++ As mentioned n the second patch, I think these should live in util/ and util/Makefile.objs should add them to util-obj-y Ideally MAINTAINERS would be updated to list someone as the official maintainer too. 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 v4 1/4] Introduce yank feature
On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> The yank feature allows to recover from hanging qemu by "yanking"
> at various parts. Other qemu systems can register themselves and
> multiple yank functions. Then all yank functions for selected
> instances can be called by the 'yank' out-of-band qmp command.
> Available instances can be queried by a 'query-yank' oob command.
>
> Signed-off-by: Lukas Straub
> ---
> qapi/misc.json | 45 +
> yank.c | 174 +
> yank.h | 67 +++
> 3 files changed, 286 insertions(+)
> create mode 100644 yank.c
> create mode 100644 yank.h
> +void yank_register_function(char *instance_name, YankFn *func, void *opaque)
> +{
> +struct YankInstance *instance;
> +struct YankFuncAndParam *entry;
> +
> +qemu_mutex_lock(&lock);
> +instance = yank_find_instance(instance_name);
> +assert(instance);
> +
> +entry = g_slice_new(struct YankFuncAndParam);
> +entry->func = func;
> +entry->opaque = opaque;
> +
> +QLIST_INSERT_HEAD(&instance->yankfns, entry, next);
> +qemu_mutex_unlock(&lock);
> +}
> +
> +void yank_unregister_function(char *instance_name, YankFn *func, void
> *opaque)
> +{
> +struct YankInstance *instance;
> +struct YankFuncAndParam *entry;
> +
> +qemu_mutex_lock(&lock);
> +instance = yank_find_instance(instance_name);
> +assert(instance);
> +
> +QLIST_FOREACH(entry, &instance->yankfns, next) {
> +if (entry->func == func && entry->opaque == opaque) {
> +QLIST_REMOVE(entry, next);
> +g_slice_free(struct YankFuncAndParam, entry);
> +qemu_mutex_unlock(&lock);
> +return;
> +}
> +}
> +
> +abort();
> +}
Since the NBD impl no longer needs to register multiple different functions
on the same insance_nane, these methods could be be simplified, to only
accept a single function, instead of keeping a whole list. This would avoid
need to pass a function into the unregister() method at all.
I don't consider this a blocker though, so you pick whether to do this
or not.
> diff --git a/yank.h b/yank.h
> new file mode 100644
> index 00..f1c8743b72
> --- /dev/null
> +++ b/yank.h
> @@ -0,0 +1,67 @@
> +
Missing license header
> +#ifndef YANK_H
> +#define YANK_H
> +
> +typedef void (YankFn) (void *opaque);
> +
> +/**
> + * yank_register_instance: Register a new instance.
> + *
> + * This registers a new instance for yanking. Must be called before any yank
> + * function is registered for this instance.
> + *
> + * This function is thread-safe.
> + *
> + * @instance_name: The globally unique name of the instance.
> + */
> +void yank_register_instance(char *instance_name);
> +
> +/**
> + * yank_unregister_instance: Unregister a instance.
> + *
> + * This unregisters a instance. Must be called only after every yank function
> + * of the instance has been unregistered.
> + *
> + * This function is thread-safe.
> + *
> + * @instance_name: The name of the instance.
> + */
> +void yank_unregister_instance(char *instance_name);
> +
> +/**
> + * yank_register_function: Register a yank function
> + *
> + * This registers a yank function. All limitations of qmp oob commands apply
> + * to the yank function as well.
> + *
> + * This function is thread-safe.
> + *
> + * @instance_name: The name of the instance
> + * @func: The yank function
> + * @opaque: Will be passed to the yank function
> + */
> +void yank_register_function(char *instance_name, YankFn *func, void *opaque);
> +
> +/**
> + * yank_unregister_function: Unregister a yank function
> + *
> + * This unregisters a yank function.
> + *
> + * This function is thread-safe.
> + *
> + * @instance_name: The name of the instance
> + * @func: func that was passed to yank_register_function
> + * @opaque: opaque that was passed to yank_register_function
> + */
> +void yank_unregister_function(char *instance_name, YankFn *func, void
> *opaque);
> +
> +/**
> + * yank_unregister_function: 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);
> +#endif
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 :|
[PATCH v4 1/4] Introduce yank feature
The yank feature allows to recover from hanging qemu by "yanking"
at various parts. Other qemu systems can register themselves and
multiple yank functions. Then all yank functions for selected
instances can be called by the 'yank' out-of-band qmp command.
Available instances can be queried by a 'query-yank' oob command.
Signed-off-by: Lukas Straub
---
qapi/misc.json | 45 +
yank.c | 174 +
yank.h | 67 +++
3 files changed, 286 insertions(+)
create mode 100644 yank.c
create mode 100644 yank.h
diff --git a/qapi/misc.json b/qapi/misc.json
index 99b90ac80b..f5228b2502 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1550,3 +1550,48 @@
##
{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
+##
+# @YankInstances:
+#
+# @instances: List of yank instances.
+#
+# Yank instances are named after the following schema:
+# "blockdev:", "chardev:" and "migration"
+#
+# Since: 5.1
+##
+{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }
+
+##
+# @yank:
+#
+# Recover from hanging qemu by yanking the specified instances.
+#
+# Takes @YankInstances as argument.
+#
+# Returns: nothing.
+#
+# Example:
+#
+# -> { "execute": "yank", "arguments": { "instances": ["blockdev:nbd0"] } }
+# <- { "return": {} }
+#
+# Since: 5.1
+##
+{ 'command': 'yank', 'data': 'YankInstances', 'allow-oob': true }
+
+##
+# @query-yank:
+#
+# Query yank instances.
+#
+# Returns: @YankInstances
+#
+# Example:
+#
+# -> { "execute": "query-yank" }
+# <- { "return": { "instances": ["blockdev:nbd0"] } }
+#
+# Since: 5.1
+##
+{ 'command': 'query-yank', 'returns': 'YankInstances', 'allow-oob': true }
diff --git a/yank.c b/yank.c
new file mode 100644
index 00..36d8139d4d
--- /dev/null
+++ b/yank.c
@@ -0,0 +1,174 @@
+/*
+ * QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub
+ *
+ * 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 "qemu/thread.h"
+#include "qemu/queue.h"
+#include "qapi/qapi-commands-misc.h"
+#include "io/channel.h"
+#include "yank.h"
+
+struct YankFuncAndParam {
+YankFn *func;
+void *opaque;
+QLIST_ENTRY(YankFuncAndParam) next;
+};
+
+struct YankInstance {
+char *name;
+QLIST_HEAD(, YankFuncAndParam) yankfns;
+QLIST_ENTRY(YankInstance) next;
+};
+
+static QemuMutex lock;
+static QLIST_HEAD(yankinst_list, YankInstance) head
+= QLIST_HEAD_INITIALIZER(head);
+
+static struct YankInstance *yank_find_instance(char *name)
+{
+struct YankInstance *tmp, *instance;
+instance = NULL;
+QLIST_FOREACH(tmp, &head, next) {
+if (!strcmp(tmp->name, name)) {
+instance = tmp;
+}
+}
+return instance;
+}
+
+void yank_register_instance(char *instance_name)
+{
+struct YankInstance *instance;
+
+qemu_mutex_lock(&lock);
+assert(!yank_find_instance(instance_name));
+
+instance = g_slice_new(struct YankInstance);
+instance->name = g_strdup(instance_name);
+QLIST_INIT(&instance->yankfns);
+QLIST_INSERT_HEAD(&head, instance, next);
+
+qemu_mutex_unlock(&lock);
+}
+
+void yank_unregister_instance(char *instance_name)
+{
+struct YankInstance *instance;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+assert(QLIST_EMPTY(&instance->yankfns));
+QLIST_REMOVE(instance, next);
+g_free(instance->name);
+g_slice_free(struct YankInstance, instance);
+
+qemu_mutex_unlock(&lock);
+}
+
+void yank_register_function(char *instance_name, YankFn *func, void *opaque)
+{
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+entry = g_slice_new(struct YankFuncAndParam);
+entry->func = func;
+entry->opaque = opaque;
+
+QLIST_INSERT_HEAD(&instance->yankfns, entry, next);
+qemu_mutex_unlock(&lock);
+}
+
+void yank_unregister_function(char *instance_name, YankFn *func, void *opaque)
+{
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+QLIST_FOREACH(entry, &instance->yankfns, next) {
+if (entry->func == func && entry->opaque == opaque) {
+QLIST_REMOVE(entry, next);
+g_slice_free(struct YankFuncAndParam, entry);
+qemu_mutex_unlock(&lock);
+return;
+}
+}
+
+abort();
+}
+
+void yank_generic_iochannel(void *opaque)
+{
+QIOChannel *ioc = QIO_CHANNEL(opaque);
+
+qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
+void qmp_yank(strList *instances, Error **errp)
+{
+strList *tmp;
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mute
