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&gt; 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&gt; > > > --- > > > 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