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
