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@redhat.com>
---
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_command.c b/src/qemu/qemu_command.c
index 6a8da1d58..5a6a671a1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3180,8 +3180,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
virQEMUCapsPtr qemuCaps)
{
size_t i, j;
+ unsigned int flags = 0;
int usbcontroller = 0;
- bool usblegacy = false;
int contOrder[] = {
/*
* List of controller types that we add commandline args for,
@@ -3216,61 +3216,14 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
if (cont->type != contOrder[j])
continue;
- /* skip USB controllers with type none.*/
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
- usbcontroller = -1; /* mark we don't want a controller */
- continue;
- }
-
- /* skip pcie-root */
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
- continue;
- }
+ if (qemuDomainDeviceDefSkipController(cont, def, &flags)) {
+ /* mark we don't want a controller */
+ if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_CONTROLLER_FLAG)
+ usbcontroller = -1;
- /* Skip pci-root, except for pSeries guests (which actually
- * support more than one PCI Host Bridge per guest) */
- if (!qemuDomainIsPSeries(def) &&
- cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
- continue;
- }
-
- /* first SATA controller on Q35 machines is implicit */
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA &&
- cont->idx == 0 && qemuDomainIsQ35(def))
- continue;
-
- /* first IDE controller is implicit on various machines */
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
- cont->idx == 0 && qemuDomainHasBuiltinIDE(def))
- continue;
-
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
- cont->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 (usblegacy) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Multiple legacy USB controllers are "
- "not supported"));
+ if (flags & QEMU_DOMAIN_DEVICE_SKIP_USB_LEGACY_FAIL_FLAG)
goto cleanup;
- }
- usblegacy = true;
+
continue;
}
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
+ */
+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;
+ }
+
+ /* 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;
+
+ 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"));
+ *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;
+
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
>---
> 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.
>+ */
>+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;
>+
>+ 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.
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.
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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
>> ---
>> 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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
>> ---
>> 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.
>
After some more thought - I'm not sure it's really clear to me what
you're driving at.
The whole point of the Skip helper was to avoid duplicating checks in
specific ValidateController{IDE|PCI|SATA|USB} helpers. If we're not
building a command line for those, then what is the purpose of going
through Validation?
I suppose I could duplicate the same checks in the helpers, but that
didn't feel right. If I take the IDE check as an example, the IDE
validation would need code like Lin Ma's patches had, e.g.:
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
/* No need to check implicit/builtin IDE controller */
if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def))
return 0;
...
Personally, trying to reduce cut-copy-paste would be a gain.
>> + */
>> +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).
>
I'm just going to drop moving the USB checks into ValidateControllerUSB
- it seems you have an idea of what you'd like to see done and I'm fine
with reviewing that when/if it shows up on the list.
The whole reason for the flags argument was because of the very specific
USB checks being done in command line building. Since you clearly
understand those better than I do, for me to make progress it's best to
just defer to you.
Updated series to be sent shortly...
John
>> +
>> + /* 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;
>> +
>> + 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.
>
> 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.
>
> 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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 06, 2017 at 10:18:53AM -0500, John Ferlan wrote:
>
>
>On 12/05/2017 09:18 AM, Ján Tomko wrote:
[...]
>>
>> 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.
>>
>
>After some more thought - I'm not sure it's really clear to me what
>you're driving at.
>
>The whole point of the Skip helper was to avoid duplicating checks in
>specific ValidateController{IDE|PCI|SATA|USB} helpers. If we're not
>building a command line for those, then what is the purpose of going
>through Validation?
>
The purpose of qemuDomain*DefValidate is to validate the domain definition,
not the command line. So it still makes sense to check if what is
provided in the domain definition matches the implicit device.
>I suppose I could duplicate the same checks in the helpers, but that
>didn't feel right. If I take the IDE check as an example, the IDE
>validation would need code like Lin Ma's patches had, e.g.:
>
> if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
> /* No need to check implicit/builtin IDE controller */
> if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def))
> return 0;
>...
>
>Personally, trying to reduce cut-copy-paste would be a gain.
Without this check, the IDE controller validation seemed a bit
confusing, since it returned an error in all the cases, leaving up to
the reader to figure out that the validate function is not called on
valid devices.
Also, since the pci(e)-root checks were skipped.
Jan
>
>>> + */
>>> +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).
>>
>
>I'm just going to drop moving the USB checks into ValidateControllerUSB
>- it seems you have an idea of what you'd like to see done and I'm fine
>with reviewing that when/if it shows up on the list.
>
>The whole reason for the flags argument was because of the very specific
>USB checks being done in command line building. Since you clearly
>understand those better than I do, for me to make progress it's best to
>just defer to you.
>
>Updated series to be sent shortly...
>
>John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.