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
<jdenemar(a)redhat.com> 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 <eperezma(a)redhat.com>
> > ---
> > 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, &reply) < 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