On Wed, Jul 20, 2022 at 4:48 PM Jiri Denemark <jdenemar@xxxxxxxxxx> wrote:
>
> On Wed, Jul 20, 2022 at 16:29:45 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark <jdenemar@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> > > > since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> > > > will return an array of error strings describing the migration blockers.
> > > > This can be used to check whether there are any devices blocking
> > > > migration, etc.
> > > >
> > > > Enable qemuMigrationSrcIsAllowed to query it.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > > > ---
> > > > v3:
> > > > * Report message with a colon.
> > > > * Report all blockers instead of only the first.
> > > > ---
> > > > src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index b12cb518ee..6ac4ef150b 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
> > > > return true;
> > > > }
> > > >
> > > > +static int
> > > > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> > > > + virDomainObj *vm,
> > > > + char ***blockers)
> > > > +{
> > > > + qemuDomainObjPrivate *priv = vm->privateData;
> > > > + int rc;
> > > > +
> > > > + qemuDomainObjEnterMonitor(driver, vm);
> > > > + rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> > > > + qemuDomainObjExitMonitor(vm);
> > > > +
> > > > + return rc;
> > > > +}
> > > >
> > > > /**
> > > > * qemuMigrationSrcIsAllowed:
> > > > @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > > > int nsnapshots;
> > > > int pauseReason;
> > > > size_t i;
> > > > + int r;
> > > > +
> > > > + /* Ask qemu if it have a migration blocker */
> > > > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > > > + g_auto(GStrv) blockers = NULL;
> > > > + r = qemuDomainGetMigrationBlockers(driver, vm, &blockers);
> > > > + if (r != 0) {
> > > > + virReportError(VIR_ERR_OPERATION_INVALID,
> > > > + _("cannot migrate domain: %s"),
> > > > + _("error getting blockers"));
> > > > + return false;
> > > > + }
> > >
> > > As mentioned in v2 review the virReportError call should be dropped as
> > > it overwrites the error reported by qemuDomainGetMigrationBlockers. That
> > > is, you can just
> > >
> > > if (qemuDomainGetMigrationBlockers(driver, vm, &blockers) < 0)
> > > return false;
> > >
> >
> > But there are a few conditions that don't report an error, like a bad
> > JSON answer. For example, if "blockers" is not an array parsing the
> > response JSON, libvirt would not print any error, isn't it?
>
> Well in qemuDomainGetMigrationBlockers you now have
>
> if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons")))
> return 0;
>
> so you would not get pass r != 0 in such case anyway. If you wanted to
> distinguish missing blocked-reasons (that is no blockers) and
> blocked-reasons which is not an array (invalid response), you would need
> to do something else:
>
> if (!(jblockers = virJSONValueObjectGet(data, "blocked-reasons")))
> return 0;
>
> if (!virJSONValueIsArray(jblockers)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("blocker-reasons is not an array"));
> return -1;
> }
>
> All this inside qemuDomainGetMigrationBlockers() to make sure the
> function reports an error when returning -1. The caller would not report
> any error anyway.
>
That's right.
Deleting error report then.
Thanks!
