Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
On Wed, Jul 20, 2022 at 1:13 PM Jiri Denemark wrote:
>
> On Wed, Jul 20, 2022 at 12:43:44 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark wrote:
> > >
> > > On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > > > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > > > migration blockers, reducing duplication.
> > > >
> > > > Signed-off-by: Eugenio Pérez
> > > > ---
> > > > src/qemu/qemu_monitor_json.c | 35 +++
> > > > src/qemu/qemu_monitor_json.h | 3 +++
> > > > 2 files changed, 38 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > > index 5e4a86e5ad..a53d721720 100644
> > > > --- a/src/qemu/qemu_monitor_json.c
> > > > +++ b/src/qemu/qemu_monitor_json.c
> > > > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> > > > return 0;
> > > > }
> > > >
> > > > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > > > +char ***blockers)
> > > > +{
> > > > +g_autoptr(virJSONValue) cmd = NULL;
> > > > +g_autoptr(virJSONValue) reply = NULL;
> > > > +virJSONValue *data;
> > > > +virJSONValue *jblockers;
> > > > +size_t i;
> > > > +
> > > > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > > > +return -1;
> > >
> > > We already have a function which calls query-migrate in JSON monitor,
> > > but I actually agree with adding this separate function as it serves a
> > > completely different purpose.
> > >
> >
> > I'm totally ok if anybody prefers to merge both too.
>
> I don't think anyone would prefer that as the existing function is a big
> beast which would get even more complicated if generalized for this use
> case.
>
> > > > +
> > > > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > > > +return -1;
> > > > +
> > > > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) <
> > > > 0)
> > > > +return -1;
> > > > +
> > > > +data = virJSONValueObjectGetObject(reply, "return");
> > > > +
> > > > +if (!(jblockers = virJSONValueObjectGetArray(data,
> > > > "blocked-reasons")))
> > > > +return -1;
> > >
> > > This is not an error (not to mention that you would return -1 without a
> > > proper error message) as missing blocked-reasons means there's no
> > > migration blocker and the domain can be migrated (as long as the
> > > QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
> > >
> >
> > Right, I'll fix it.
> >
> > Regarding the message on failure, a lot of the functions in this file
> > returns -1 without a message. How can I print it properly here or
> > propagate to the caller?
>
> Well, returning -1 is fine if the function called
> (qemuMonitorJSONCommand, qemuMonitorJSONCheckReply) already reports an
> error. But this is not the case of virJSONValueObjectGetArray. Reporting
> an error would be done just by calling virReportError() here instead of
> having it in the caller. Anyway, nothing to worry about here as you
> will be returning 0.
>
Got it.
Thanks!
> Jirka
>
Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
On Wed, Jul 20, 2022 at 12:43:44 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > > migration blockers, reducing duplication.
> > >
> > > Signed-off-by: Eugenio Pérez
> > > ---
> > > src/qemu/qemu_monitor_json.c | 35 +++
> > > src/qemu/qemu_monitor_json.h | 3 +++
> > > 2 files changed, 38 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index 5e4a86e5ad..a53d721720 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> > > return 0;
> > > }
> > >
> > > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > > +char ***blockers)
> > > +{
> > > +g_autoptr(virJSONValue) cmd = NULL;
> > > +g_autoptr(virJSONValue) reply = NULL;
> > > +virJSONValue *data;
> > > +virJSONValue *jblockers;
> > > +size_t i;
> > > +
> > > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > > +return -1;
> >
> > We already have a function which calls query-migrate in JSON monitor,
> > but I actually agree with adding this separate function as it serves a
> > completely different purpose.
> >
>
> I'm totally ok if anybody prefers to merge both too.
I don't think anyone would prefer that as the existing function is a big
beast which would get even more complicated if generalized for this use
case.
> > > +
> > > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > > +return -1;
> > > +
> > > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> > > +return -1;
> > > +
> > > +data = virJSONValueObjectGetObject(reply, "return");
> > > +
> > > +if (!(jblockers = virJSONValueObjectGetArray(data,
> > > "blocked-reasons")))
> > > +return -1;
> >
> > This is not an error (not to mention that you would return -1 without a
> > proper error message) as missing blocked-reasons means there's no
> > migration blocker and the domain can be migrated (as long as the
> > QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
> >
>
> Right, I'll fix it.
>
> Regarding the message on failure, a lot of the functions in this file
> returns -1 without a message. How can I print it properly here or
> propagate to the caller?
Well, returning -1 is fine if the function called
(qemuMonitorJSONCommand, qemuMonitorJSONCheckReply) already reports an
error. But this is not the case of virJSONValueObjectGetArray. Reporting
an error would be done just by calling virReportError() here instead of
having it in the caller. Anyway, nothing to worry about here as you
will be returning 0.
Jirka
Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark wrote:
>
> On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> > This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> > migration blockers, reducing duplication.
> >
> > Signed-off-by: Eugenio Pérez
> > ---
> > src/qemu/qemu_monitor_json.c | 35 +++
> > src/qemu/qemu_monitor_json.h | 3 +++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 5e4a86e5ad..a53d721720 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> > return 0;
> > }
> >
> > +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> > +char ***blockers)
> > +{
> > +g_autoptr(virJSONValue) cmd = NULL;
> > +g_autoptr(virJSONValue) reply = NULL;
> > +virJSONValue *data;
> > +virJSONValue *jblockers;
> > +size_t i;
> > +
> > +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> > +return -1;
>
> We already have a function which calls query-migrate in JSON monitor,
> but I actually agree with adding this separate function as it serves a
> completely different purpose.
>
I'm totally ok if anybody prefers to merge both too.
> > +
> > +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > +return -1;
> > +
> > +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> > +return -1;
> > +
> > +data = virJSONValueObjectGetObject(reply, "return");
> > +
> > +if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> > +return -1;
>
> This is not an error (not to mention that you would return -1 without a
> proper error message) as missing blocked-reasons means there's no
> migration blocker and the domain can be migrated (as long as the
> QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
>
Right, I'll fix it.
Regarding the message on failure, a lot of the functions in this file
returns -1 without a message. How can I print it properly here or
propagate to the caller?
> I think we should return 0 and set *blockers = NULL in this case.
>
Sure, I'll change that.
> > +
> > +/* NULL terminated array */
>
> This comment would be better as part of a proper documentation above the
> function.
>
I'll move to the function doc.
Thanks!
> > +*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
> > +for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
> > +virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
> > +const char *blocker = virJSONValueGetString(jblocker);
> > +
> > +(*blockers)[i] = g_strdup(blocker);
> > +}
> > +
> > +return 0;
> > +}
> > +
> > int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
> > {
> > g_autoptr(virJSONValue) cmd =
> > qemuMonitorJSONMakeCommand("migrate_cancel", NULL);
>
> Jirka
>
Re: [libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
On Wed, Jul 20, 2022 at 11:11:51 +0200, Eugenio Pérez wrote:
> This will allow qemuMigrationSrcIsAllowed to dynamically ask for
> migration blockers, reducing duplication.
>
> Signed-off-by: Eugenio Pérez
> ---
> src/qemu/qemu_monitor_json.c | 35 +++
> src/qemu/qemu_monitor_json.h | 3 +++
> 2 files changed, 38 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5e4a86e5ad..a53d721720 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
> return 0;
> }
>
> +int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
> +char ***blockers)
> +{
> +g_autoptr(virJSONValue) cmd = NULL;
> +g_autoptr(virJSONValue) reply = NULL;
> +virJSONValue *data;
> +virJSONValue *jblockers;
> +size_t i;
> +
> +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
> +return -1;
We already have a function which calls query-migrate in JSON monitor,
but I actually agree with adding this separate function as it serves a
completely different purpose.
> +
> +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> +return -1;
> +
> +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> +return -1;
> +
> +data = virJSONValueObjectGetObject(reply, "return");
> +
> +if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> +return -1;
This is not an error (not to mention that you would return -1 without a
proper error message) as missing blocked-reasons means there's no
migration blocker and the domain can be migrated (as long as the
QEMU_CAPS_MIGRATION_BLOCKED_REASONS capability is set).
I think we should return 0 and set *blockers = NULL in this case.
> +
> +/* NULL terminated array */
This comment would be better as part of a proper documentation above the
function.
> +*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
> +for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
> +virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
> +const char *blocker = virJSONValueGetString(jblocker);
> +
> +(*blockers)[i] = g_strdup(blocker);
> +}
> +
> +return 0;
> +}
> +
> int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
> {
> g_autoptr(virJSONValue) cmd =
> qemuMonitorJSONMakeCommand("migrate_cancel", NULL);
Jirka
[libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers
This will allow qemuMigrationSrcIsAllowed to dynamically ask for
migration blockers, reducing duplication.
Signed-off-by: Eugenio Pérez
---
src/qemu/qemu_monitor_json.c | 35 +++
src/qemu/qemu_monitor_json.h | 3 +++
2 files changed, 38 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5e4a86e5ad..a53d721720 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3338,6 +3338,41 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon,
return 0;
}
+int qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValue *data;
+virJSONValue *jblockers;
+size_t i;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
+return -1;
+
+data = virJSONValueObjectGetObject(reply, "return");
+
+if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
+return -1;
+
+/* NULL terminated array */
+*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1);
+for (i = 0; i < virJSONValueArraySize(jblockers); i++) {
+virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i);
+const char *blocker = virJSONValueGetString(jblocker);
+
+(*blockers)[i] = g_strdup(blocker);
+}
+
+return 0;
+}
+
int qemuMonitorJSONMigrateCancel(qemuMonitor *mon)
{
g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("migrate_cancel",
NULL);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 2759566892..e4c65e250e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon,
unsigned int flags,
const char *uri);
int
+qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon,
+char ***blockers);
+int
qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon,
bool *spice_migrated);
--
2.31.1
