[libvirt PATCH v2 2/5] qemu_monitor_json: Add qemuMonitorJSONGetMigrationBlockers

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 12:14 PM Jiri Denemark <jdenemar at 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 at 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.

> > +
> > +    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?

> 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
>