Re: [libvirt] [PATCH 07/22] qemu_capabilities: Introduce virQEMUCapsCacheLookupDefault

On 05/16/2018 04:39 AM, Jiri Denemark wrote: > virConnectGetDomainCapabilities needs to lookup QEMU capabilities > matching a specified binary, architecture, virt type, and machine type > while using default values when any of the parameters are not provided > by the user. Let's extract the lookup code into > virQEMUCapsCacheLookupDefault to make it reusable. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++++ > src/qemu/qemu_capabilities.h | 8 +++ > src/qemu/qemu_driver.c | 86 ++++--------------------- > 3 files changed, 137 insertions(+), 75 deletions(-) > [...] > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9037818e2a..6c086b9ef8 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -19249,10 +19249,9 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, > char *ret = NULL; > virQEMUDriverPtr driver = conn->privateData; > virQEMUCapsPtr qemuCaps = NULL; > - int virttype = VIR_DOMAIN_VIRT_NONE; > - virDomainVirtType capsType; > + virArch arch; > + virDomainVirtType virttype; > virDomainCapsPtr domCaps = NULL; > - int arch = virArchFromHost(); /* virArch */ > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > > @@ -19266,80 +19265,17 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, > if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > goto cleanup; > > - if (virttype_str && > - (virttype = virDomainVirtTypeFromString(virttype_str)) < 0) { > - virReportError(VIR_ERR_INVALID_ARG, > - _("unknown virttype: %s"), > - virttype_str); > + qemuCaps = virQEMUCapsCacheLookupDefault(driver->qemuCapsCache, > + emulatorbin, > + arch_str, > + virttype_str, > + machine, > + &arch, &virttype, &machine); > + if (!qemuCaps) > goto cleanup; > - } > > - if (arch_str && (arch = virArchFromString(arch_str)) == VIR_ARCH_NONE) { > - virReportError(VIR_ERR_INVALID_ARG, > - _("unknown architecture: %s"), > - arch_str); > - goto cleanup; > - } > - > - if (emulatorbin) { > - virArch arch_from_caps; > - > - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, > - emulatorbin))) > - goto cleanup; > - > - arch_from_caps = virQEMUCapsGetArch(qemuCaps); > - > - if (arch_from_caps != arch && > - !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) || > - (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) || > - (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) || > - (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) { > - virReportError(VIR_ERR_INVALID_ARG, > - _("architecture from emulator '%s' doesn't " > - "match given architecture '%s'"), > - virArchToString(arch_from_caps), > - virArchToString(arch)); > - goto cleanup; > - } Are all these checks necessary? Can't you get away with just checking arch_from_caps != arch?