On Wed, 2016-03-30 at 13:26 -0400, John Ferlan wrote:
> Is there a way to set a capabilities bit to determine if the
> "query-gic-capabilities" command exists? Perhaps virQEMUCapsCommands?
> Changes a check later on in qemu_monitor_json.c
Sure, but would that be useful?
We should already be handling the case where the QMP command is
not available gracefully, eg. returning an empty list of GIC
capabilities. How would adding a capabilities bit improve the
situation?
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 4b1e750..e54208a 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -354,6 +354,9 @@ struct _virQEMUCaps {
> > char **machineTypes;
> > char **machineAliases;
> > unsigned int *machineMaxCpus;
> > +
> > + size_t ngicCapabilities;
> > + virGICCapability *gicCapabilities;
>
> Any reason to not use virGICCapabilityPtr ?
virGICCapabilityPtr would be appropriate if this was a pointer
to a single virGICCapability instance; here we have an array
of virGICCapability instances, so I think virGICCapability* is
more suitable.
Same goes for the other instances you've noted later on.
> > struct virQEMUCapsSearchData {
> > @@ -2690,6 +2693,22 @@ virQEMUCapsProbeQMPMigrationCapabilities(virQEMUCapsPtr qemuCaps,
> > return 0;
> > }
> >
> May as well go with the 2 empty lines between functions here.
I really wish we had a standard for how many empty lines there
should be between functions. It seems to vary wildly between
modules, and even inside the same module :(
> Need some intro comments, arguments, returns, etc. Not every command
> does it, but let's try to add some
Right, I'll add some.
> > + int ncaps;
> > +
> > + if ((ncaps = qemuMonitorGetGICCapabilities(mon, &caps)) < 0)
> > + return -1;
> > +
> > + qemuCaps->gicCapabilities = caps;
> > + qemuCaps->ngicCapabilities = ncaps;
>
> You will need to VIR_FREE(qemuCaps->gicCapabilities) in
> virQEMUCapsDispose and virQEMUCapsReset (looked up where machineTypes
> and machineAliases free'd memory).
Good catch.
> > +
> > + return 0;
> > +}
> > +
> > int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps,
> > qemuMonitorPtr mon)
> > {
> > @@ -3410,6 +3429,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> > if (virQEMUCapsProbeQMPMigrationCapabilities(qemuCaps, mon) < 0)
> > goto cleanup;
> >
> > + /* GIC capabilities, eg. available GIC versions */
> > + if (ARCH_IS_ARM(qemuCaps->arch) &&
>
> I note that patch 5 only fills in the domain feature caps based on
> VIR_ARCH_ARMV7L and VIR_ARCH_AARCH64... Does that perhaps mean we need
> some sort of "second" ARCH_IS_ARM type macro? There are some other
> places where the two are checked...
Again, good catch. There's no reason to be inconsistent here.
As for adding a new ARCH_IS_*() macro, I'm honestly not sure
what such a macro would look like. I'll try to poke around and
come up with some guidelines on which sub-architectures we
should check for, instead of just going with whatever's
already there.
> > + size_t i;
> > + ssize_t n;
> > +
> > + *capabilities = NULL;
> > +
> > + if (!(cmd = qemuMonitorJSONMakeCommand("query-gic-capabilities",
> > + NULL)))
> > + return -1;
> > +
> > + ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> > +
> > + if (ret == 0) {
> > + if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
> > + goto cleanup;
>
> One would hope that if we checked the capability to have the command
> before calling this, then this check wouldn't be necessary.
>
> In any case, if it is necessary to keep, then we don't fail back to the
> original caller (virQEMUCapsProbeQMPGICCapabilities). Not a problem I
> suppose, the caller would get a 'ncaps == 0' and "caps == NULL"...
'caps == NULL' and 'ncaps == 0' is perfectly fine, that's how
the caller can tell the QEMU binary has no GIC capabilities.
> > + ret = qemuMonitorJSONCheckError(cmd, reply);
> > + }
> > +
> > + if (ret < 0)
> > + goto cleanup;
> > +
> > + ret = -1;
> > +
> > + if (!(caps = virJSONValueObjectGetArray(reply, "return")) ||
> > + (n = virJSONValueArraySize(caps)) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("missing GIC capabilities"));
> > + goto cleanup;
> > + }
> > +
>
> Should n == 0 here? Would that be an error? e.g., change the < 0 to <=
> 0 above.
Well, 'n < 0' means that virJSONValueArraySize() failed, ie.
the QMP command didn't return an array. 'n == 0' means that
the returned array is empty, which is perfectly fine.
But I realize now that such case is not handled properly: we
should return NULL caps, instead we return an empty 1-element
array.
> > + if (VIR_ALLOC_N(list, n + 1) < 0)
> > + goto cleanup;
>
> why + 1? (it's not done in patch 6, virQEMUCapsLoadCache)
Yeah, having '+ 1' here is just a waste of memory.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list