Re: [PATCH 02/17] qemu: Introduce qemuDomainDeviceDefSkipController

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/05/2017 09:18 AM, Ján Tomko wrote:
> On Mon, Dec 04, 2017 at 08:38:52PM -0500, John Ferlan wrote:
>> In order to be able to reuse some code for both controller def
>> validation and command line building, create a convenience helper
>> that will perform the "skip" avoidance checks. It will also set
>> some flags to allow the caller to specifically skip (or fail)
>> depending on the result of the flag (as opposed to building up
>> some ever growing list of variables).
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_command.c | 61 +++++------------------------------
>> src/qemu/qemu_domain.c  | 84
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>> src/qemu/qemu_domain.h  | 12 +++++++
>> 3 files changed, 102 insertions(+), 55 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 569bbd29e..d4c7674c0 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3884,10 +3884,92 @@ qemuDomainDeviceDefValidateDisk(const
>> virDomainDiskDef *disk)
>> }
>>
>>
>> +/**
>> + * qemuDomainDeviceDefSkipController:
>> + * @controller: Controller to check
>> + * @def: Domain definition
>> + * @flags: qemuDomainDeviceSkipFlags to set if specific condition found
>> + *
>> + * Returns true if this controller can be skipped for command line
>> generation
>> + * or device validation
> 
> We should not skip validation for implicit devices. Some of the checks
> added later are for implicit devices (PCI root, SATA controller).
> 
> It would be enough to split out the logic of figuring out whether the
> controller is implicit out of command line generation.
> 

Obviously the "goal" was to avoid the same checks in Validation that
we're avoiding in command line building; however, if there are things
that need to be checked in Validation before what gets checked here,
then I could see that being "extra".

What I was trying to avoid was duplication of specific checks prior to
Validation that were being avoided anyways for command line building. As
in - we don't build that on the command line, so why bother checking
during validation.

>> + */
>> +bool
>> +qemuDomainDeviceDefSkipController(const virDomainControllerDef
>> *controller,
>> +                                  const virDomainDef *def,
>> +                                  unsigned int *flags)
>> +{
>> +    /* skip USB controllers with type none.*/
>> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
>> +        controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
>> +        *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG;
>> +        return true;
>> +    }
> 
> Simply returning true here (no USB controller is implicit) should
> suffice. The caller can figure out whether USB_NONE is present
> by going through the controllers array again (via virDomainDefHasUSB).
> 
>> +
>> +    /* skip pcie-root */
>> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +        controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>> +        return true;
>> +
>> +    /* Skip pci-root, except for pSeries guests (which actually
>> +     * support more than one PCI Host Bridge per guest) */
>> +    if (!qemuDomainIsPSeries(def) &&
>> +        controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +        controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
>> +        return true;
>> +
>> +    /* first SATA controller on Q35 machines is implicit */
>> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA &&
>> +        controller->idx == 0 && qemuDomainIsQ35(def))
>> +        return true;
>> +
>> +    /* first IDE controller is implicit on various machines */
>> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
>> +        controller->idx == 0 && qemuDomainHasBuiltinIDE(def))
>> +        return true;

This one is the initial reason I figured it would be better to have a
Skip helper as opposed to "copying" the same check in the ValidateIDE
code that we'd have in the command line building code.

Then the subsequent ones made the ValidateUSB *so much* easier.

>> +
>> +    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
>> +        controller->model == -1 &&
>> +        !qemuDomainIsQ35(def) &&
>> +        !qemuDomainIsVirt(def)) {
>> +
>> +        /* An appropriate default USB controller model should already
>> +         * have been selected in qemuDomainDeviceDefPostParse(); if
>> +         * we still have no model by now, we have to fall back to the
>> +         * legacy USB controller.
>> +         *
>> +         * Note that we *don't* want to end up with the legacy USB
>> +         * controller for q35 and virt machines, so we go ahead and
>> +         * fail in qemuBuildControllerDevStr(); on the other hand,
>> +         * for s390 machines we want to ignore any USB controller
>> +         * (see 548ba43028 for the full story), so we skip
>> +         * qemuBuildControllerDevStr() but we don't ultimately end
>> +         * up adding the legacy USB controller */
>> +        if (*flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("Multiple legacy USB controllers are "
>> +                             "not supported"));
> 
> A function returning bool where both options are valid should not be
> setting errors.
> 

Yeah - I figured this may cause a few eyebrows to raise.

> For this error, validate can go through all the controllers. Command
> line generator should not care and we can get rid of the remaining two
> flags as their only purpose is to save us multiple passes through
> the controllers field.
> 
>> +            *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG;
>> +        }
>> +        *flags |= QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG;
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +
>> static int
>> qemuDomainDeviceDefValidateController(const virDomainControllerDef
>> *controller,
>> -                                      const virDomainDef *def
>> ATTRIBUTE_UNUSED)
>> +                                      const virDomainDef *def)
>> {
>> +    unsigned int flags = 0;
>> +
>> +    if (qemuDomainDeviceDefSkipController(controller, def, &flags))
>> +        return 0;
>> +
>> +    if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG)
>> +        return -1;
> 
> To possibly set this flag, SkipController must be called with non-zero
> flags, so this would be dead code.
> 
> I'd rather go through def->controllers again to look for another legacy
> controller, just for the purpose of validation.
> 

OK - I'll try to figure out some sort of happy medium. It obviously
could affect subsequent patches in the series.

John

> Jan
> 
>> +
>>     switch ((virDomainControllerType) controller->type) {
>>     case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index c33af3671..5c9c55d38 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -999,4 +999,16 @@ qemuDomainPrepareDiskSource(virConnectPtr conn,
>>                             qemuDomainObjPrivatePtr priv,
>>                             virQEMUDriverConfigPtr cfg);
>>
>> +typedef enum {
>> +    QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG = (1 << 0),
>> +    QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FLAG = (1 << 1),
>> +    QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG = (1 << 2),
>> +} qemuDomainDeviceSkipFlags;
>> +
>> +bool
>> +qemuDomainDeviceDefSkipController(const virDomainControllerDef
>> *controller,
>> +                                  const virDomainDef *def,
>> +                                  unsigned int *flags);
>> +
>> +
>> #endif /* __QEMU_DOMAIN_H__ */
>> -- 
>> 2.13.6
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux