9:58 a.m.
The purpose of this series is just to eliminate the need to "count
indexes" when adding controllers.
Patch 4/4 is the meat of this series. 2/4 and 3/4 are code movement to
enable 4/4, and 1/4 is a small code simplification that I noticed
while testing the rest.
Laine Stump (4):
qemu: simplify addition of USB controller in qemuParseCommandLine
conf/qemu: make IS_USB2_CONTROLLER globally available
conf: make virDomainControllerFindUnusedIndex() more generally usable
conf: permit auto-assignment of controller indexes
docs/schemas/domaincommon.rng | 8 ++--
src/conf/domain_conf.c | 92 ++++++++++++++++++++++++++++++++++++------
src/conf/domain_conf.h | 7 ++++
src/qemu/qemu_domain_address.c | 7 ----
src/qemu/qemu_parse_command.c | 8 +---
5 files changed, 92 insertions(+), 30 deletions(-)
--
2.5.5
9:58 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: simplify addition of USB controller in qemuParseCommandLine
virDomainDefAddUSBController() does everything that the multiple lines
of code were doing.
---
src/qemu/qemu_parse_command.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 7ce90f9..23b0159 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2350,14 +2350,8 @@ qemuParseCommandLine(virCapsPtr caps,
WANT_VALUE();
/* ignore, generted on the fly */
} else if (STREQ(arg, "-usb")) {
- virDomainControllerDefPtr ctldef;
- ctldef = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB);
- if (!ctldef)
+ if (virDomainDefAddUSBController(def, -1, -1) < 0)
goto error;
- if (virDomainControllerInsert(def, ctldef) < 0) {
- virDomainControllerDefFree(ctldef);
- goto error;
- }
} else if (STREQ(arg, "-pidfile")) {
WANT_VALUE();
if (pidfile)
--
2.5.5
9:52 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: simplify addition of USB controller in qemuParseCommandLine
On 05/11/2016 10:58 AM, Laine Stump wrote:
virDomainDefAddUSBController() does everything that the multiple
lines
of code were doing.
---
src/qemu/qemu_parse_command.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 7ce90f9..23b0159 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2350,14 +2350,8 @@ qemuParseCommandLine(virCapsPtr caps,
WANT_VALUE();
/* ignore, generted on the fly */
} else if (STREQ(arg, "-usb")) {
- virDomainControllerDefPtr ctldef;
- ctldef = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB);
- if (!ctldef)
+ if (virDomainDefAddUSBController(def, -1, -1) < 0)
goto error;
- if (virDomainControllerInsert(def, ctldef) < 0) {
- virDomainControllerDefFree(ctldef);
- goto error;
- }
} else if (STREQ(arg, "-pidfile")) {
WANT_VALUE();
if (pidfile)
ACK, and could be pushed independent of the other patches
- Cole
9:58 a.m.
New subject: [libvirt] [PATCH 2/4] conf/qemu: make IS_USB2_CONTROLLER globally available
IS_USB2_CONTROLLER() is useful in more places aside from just when
assigning PCI addresses in QEMU, and is checking for enum values that
are all defined in conf/domain_conf.h anyway, so define it there too.
---
src/conf/domain_conf.h | 7 +++++++
src/qemu/qemu_domain_address.c | 7 -------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b9e696d..40174b5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -808,6 +808,13 @@ typedef enum {
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
} virDomainControllerModelUSB;
+# define IS_USB2_CONTROLLER(ctrl) \
+ (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
+ ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
+ (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1 || \
+ (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2 || \
+ (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3))
+
typedef struct _virDomainVirtioSerialOpts virDomainVirtioSerialOpts;
typedef virDomainVirtioSerialOpts *virDomainVirtioSerialOptsPtr;
struct _virDomainVirtioSerialOpts {
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 9c8c262..f0b3062 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -973,13 +973,6 @@ qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus)
}
-#define IS_USB2_CONTROLLER(ctrl) \
- (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
- ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
- (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1 || \
- (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2 || \
- (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3))
-
/*
* This assigns static PCI slots to all configured devices.
* The ordering here is chosen to match the ordering used
--
2.5.5
9:58 a.m.
New subject: [libvirt] [PATCH 3/4] conf: make virDomainControllerFindUnusedIndex() more generally usable
This patch declares virDomainControllerFindUnusedIndex() near the top
of its file so that it can be used by other functions higher up in the
file, as well as making the DomainDef a const* so that functions which
only have a const* to the domain can use it.
---
src/conf/domain_conf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 571b7bf..728949b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -830,6 +830,7 @@ static virClassPtr virDomainObjClass;
static virClassPtr virDomainXMLOptionClass;
static void virDomainObjDispose(void *obj);
static void virDomainXMLOptionClassDispose(void *obj);
+static int virDomainControllerFindUnusedIndex(virDomainDef const *def, int type);
static int virDomainObjOnceInit(void)
{
@@ -13684,7 +13685,7 @@ virDomainControllerFind(const virDomainDef *def,
static int
-virDomainControllerFindUnusedIndex(virDomainDefPtr def, int type)
+virDomainControllerFindUnusedIndex(virDomainDef const *def, int type)
{
int idx = 0;
--
2.5.5
9:58 a.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
Hand-entering indexes for 20 PCI controllers is not as tedious as
manually determining and entering their PCI addresses, but it's still
annoying, and the algorithm for determining the proper index is
incredibly simple (in all cases except one) - just pick the lowest
unused index.
The one exception is USB2 controllers because multiple controllers in
the same group have the same index. For these we look to see if 1) the
most recently added USB controller is also a USB2 controller, and 2)
the group *that* controller belongs to doesn't yet have a controller
of the exact model we're just now adding - if both are true, the new
controller gets the same index, but in all other cases we just assign
the lowest unused index.
With this patch in place and combined with the automatic PCI address
assignment, we can define a PCIe switch with several ports like this:
<controller type='pci' model='pcie-root-port'/>
<controller type='pci' model='pcie-switch-upstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
...
These will each get a unique index, and PCI addresses that connect
them together appropriately with no pesky numbers required.
---
docs/schemas/domaincommon.rng | 8 ++--
src/conf/domain_conf.c | 89 +++++++++++++++++++++++++++++++++++++------
2 files changed, 82 insertions(+), 15 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 273715c..6abd771 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1701,9 +1701,11 @@
</define>
<define name="controller">
<element name="controller">
- <attribute name="index">
- <ref name="unsignedInt"/>
- </attribute>
+ <optional>
+ <attribute name="index">
+ <ref name="unsignedInt"/>
+ </attribute>
+ </optional>
<interleave>
<optional>
<ref name="alias"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 728949b..85ffcac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7850,6 +7850,7 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
static virDomainControllerDefPtr
virDomainControllerDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
+ virDomainDef const *dom,
unsigned int flags)
{
virDomainControllerDefPtr def = NULL;
@@ -7890,6 +7891,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
if (!(def = virDomainControllerDefNew(type)))
goto error;
+ model = virXMLPropString(node, "model");
+ if (model) {
+ if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0)
{
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unknown model type '%s'"), model);
+ goto error;
+ }
+ }
+
idx = virXMLPropString(node, "index");
if (idx) {
if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 ||
@@ -7898,15 +7908,70 @@ virDomainControllerDefParseXML(xmlNodePtr node,
_("Cannot parse controller index %s"), idx);
goto error;
}
- }
+ } else {
+ /* When no index is provided, find one by looking at what
+ * indexes are already used for this controller type in the
+ * domain.
+ */
+ virDomainControllerDefPtr prev = NULL;
+
+ if (IS_USB2_CONTROLLER(def)) {
+ /* Attempt to put new USB2 controller at the same index as
+ * other USB2 controllers, but only if this appears to be
+ * the intent. To prove intent: The last USB controller
+ * already on the list must also be a USB2 controller, and
+ * there must not yet be a controller with the exact same
+ * model of this one and the same index as the previously
+ * added controller (e.g., if this controller is a UHCI1,
+ * then the previous controller must be an EHCI1 or a
+ * UHCI[23], and there must not already be a UHCI1
+ * controller with the same index as the previous
+ * controller). If all of these are satisfied, set this
+ * controller to the same index as the previous
+ * controller. NB: we are of course assuming that this
+ * new controller is about to be appended to the domain's
+ * controller list.
+ */
+ int prevIdx;
+ size_t i;
- model = virXMLPropString(node, "model");
- if (model) {
- if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0)
{
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unknown model type '%s'"), model);
- goto error;
+ if (dom->ncontrollers) {
+ prevIdx = dom->ncontrollers - 1;
+ while (prevIdx >= 0 &&
+ dom->controllers[prevIdx]->type !=
VIR_DOMAIN_CONTROLLER_TYPE_USB)
+ prevIdx--;
+ if (prevIdx >= 0)
+ prev = dom->controllers[prevIdx];
+ }
+ /* if the last USB controller isn't USB2, that breaks
+ * the chain, so we need a new index for this new
+ * controller
+ */
+ if (!IS_USB2_CONTROLLER(prev))
+ prev = NULL;
+
+ /* if prev != NULL, we've found a potential index to
+ * use. Now we need to make sure that index isn't
+ * already used by an existing USB2 controller of the
+ * same model as this new one.
+ */
+ for (i = 0; prev && i < dom->ncontrollers; i++) {
+ if (dom->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB
&&
+ dom->controllers[i]->idx == prev->idx &&
+ dom->controllers[i]->model == def->model) {
+ /* already have a controller of this model
+ * with the proposed index, so we need to move
+ * to a new index
+ */
+ prev = NULL;
+ }
+ }
+ if (prev)
+ def->idx = prev->idx;
}
+ /* if none of the above applied, prev will be NULL */
+ if (!prev)
+ def->idx = virDomainControllerFindUnusedIndex(dom, def->type);
}
cur = node->children;
@@ -12958,7 +13023,7 @@ virDomainDeviceDefParse(const char *xmlStr,
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
if (!(dev->data.controller = virDomainControllerDefParseXML(node, ctxt,
- flags)))
+ def, flags)))
goto error;
break;
case VIR_DOMAIN_DEVICE_GRAPHICS:
@@ -16157,10 +16222,10 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
for (i = 0; i < n; i++) {
- virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i],
- ctxt,
- flags);
- if (!controller)
+ virDomainControllerDefPtr controller;
+
+ if (!(controller = virDomainControllerDefParseXML(nodes[i], ctxt,
+ def, flags)))
goto error;
/* sanitize handling of "none" usb controller */
--
2.5.5
9:56 a.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/11/2016 10:58 AM, Laine Stump wrote:
Hand-entering indexes for 20 PCI controllers is not as tedious as
manually determining and entering their PCI addresses, but it's still
annoying, and the algorithm for determining the proper index is
incredibly simple (in all cases except one) - just pick the lowest
unused index.
The one exception is USB2 controllers because multiple controllers in
the same group have the same index. For these we look to see if 1) the
most recently added USB controller is also a USB2 controller, and 2)
the group *that* controller belongs to doesn't yet have a controller
of the exact model we're just now adding - if both are true, the new
controller gets the same index, but in all other cases we just assign
the lowest unused index.
With this patch in place and combined with the automatic PCI address
assignment, we can define a PCIe switch with several ports like this:
<controller type='pci' model='pcie-root-port'/>
<controller type='pci' model='pcie-switch-upstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
<controller type='pci' model='pcie-switch-downstream-port'/>
...
These will each get a unique index, and PCI addresses that connect
them together appropriately with no pesky numbers required.
---
docs/schemas/domaincommon.rng | 8 ++--
src/conf/domain_conf.c | 89 +++++++++++++++++++++++++++++++++++++------
2 files changed, 82 insertions(+), 15 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 273715c..6abd771 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1701,9 +1701,11 @@
</define>
<define name="controller">
<element name="controller">
- <attribute name="index">
- <ref name="unsignedInt"/>
- </attribute>
+ <optional>
+ <attribute name="index">
+ <ref name="unsignedInt"/>
+ </attribute>
+ </optional>
<interleave>
<optional>
<ref name="alias"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 728949b..85ffcac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7850,6 +7850,7 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr
def,
static virDomainControllerDefPtr
virDomainControllerDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
+ virDomainDef const *dom,
unsigned int flags)
{
virDomainControllerDefPtr def = NULL;
@@ -7890,6 +7891,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
if (!(def = virDomainControllerDefNew(type)))
goto error;
+ model = virXMLPropString(node, "model");
+ if (model) {
+ if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0)
{
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unknown model type '%s'"), model);
+ goto error;
+ }
+ }
+
idx = virXMLPropString(node, "index");
if (idx) {
if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 ||
@@ -7898,15 +7908,70 @@ virDomainControllerDefParseXML(xmlNodePtr node,
_("Cannot parse controller index %s"), idx);
goto error;
}
- }
+ } else {
+ /* When no index is provided, find one by looking at what
+ * indexes are already used for this controller type in the
+ * domain.
+ */
+ virDomainControllerDefPtr prev = NULL;
+
+ if (IS_USB2_CONTROLLER(def)) {
+ /* Attempt to put new USB2 controller at the same index as
+ * other USB2 controllers, but only if this appears to be
+ * the intent. To prove intent: The last USB controller
+ * already on the list must also be a USB2 controller, and
+ * there must not yet be a controller with the exact same
+ * model of this one and the same index as the previously
+ * added controller (e.g., if this controller is a UHCI1,
+ * then the previous controller must be an EHCI1 or a
+ * UHCI[23], and there must not already be a UHCI1
+ * controller with the same index as the previous
+ * controller). If all of these are satisfied, set this
+ * controller to the same index as the previous
+ * controller. NB: we are of course assuming that this
+ * new controller is about to be appended to the domain's
+ * controller list.
+ */
+ int prevIdx;
+ size_t i;
- model = virXMLPropString(node, "model");
- if (model) {
- if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0)
{
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unknown model type '%s'"), model);
- goto error;
+ if (dom->ncontrollers) {
+ prevIdx = dom->ncontrollers - 1;
+ while (prevIdx >= 0 &&
+ dom->controllers[prevIdx]->type !=
VIR_DOMAIN_CONTROLLER_TYPE_USB)
+ prevIdx--;
+ if (prevIdx >= 0)
+ prev = dom->controllers[prevIdx];
+ }
+ /* if the last USB controller isn't USB2, that breaks
+ * the chain, so we need a new index for this new
+ * controller
+ */
+ if (!IS_USB2_CONTROLLER(prev))
+ prev = NULL;
+
+ /* if prev != NULL, we've found a potential index to
+ * use. Now we need to make sure that index isn't
+ * already used by an existing USB2 controller of the
+ * same model as this new one.
+ */
+ for (i = 0; prev && i < dom->ncontrollers; i++) {
+ if (dom->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB
&&
+ dom->controllers[i]->idx == prev->idx &&
+ dom->controllers[i]->model == def->model) {
+ /* already have a controller of this model
+ * with the proposed index, so we need to move
+ * to a new index
+ */
+ prev = NULL;
+ }
+ }
+ if (prev)
+ def->idx = prev->idx;
}
+ /* if none of the above applied, prev will be NULL */
+ if (!prev)
+ def->idx = virDomainControllerFindUnusedIndex(dom, def->type);
}
cur = node->children;
@@ -12958,7 +13023,7 @@ virDomainDeviceDefParse(const char *xmlStr,
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
if (!(dev->data.controller = virDomainControllerDefParseXML(node, ctxt,
- flags)))
+ def, flags)))
goto error;
break;
case VIR_DOMAIN_DEVICE_GRAPHICS:
@@ -16157,10 +16222,10 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
for (i = 0; i < n; i++) {
- virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i],
- ctxt,
- flags);
- if (!controller)
+ virDomainControllerDefPtr controller;
+
+ if (!(controller = virDomainControllerDefParseXML(nodes[i], ctxt,
+ def, flags)))
goto error;
/* sanitize handling of "none" usb controller */
I agree with the goal of the patch, but I think all the index assignment code
should be moved to somewhere in the PostParse call path. The fact that the
controller ParseXML now needs to act on the entire domain def is a giveaway
that it's in the wrong place.
Also this seems like it should have test suite changes too
Thanks,
Cole
1:29 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/14/2016 10:56 AM, Cole Robinson wrote:
On 05/11/2016 10:58 AM, Laine Stump wrote:
> Hand-entering indexes for 20 PCI controllers is not as tedious as
> manually determining and entering their PCI addresses, but it's still
> annoying, and the algorithm for determining the proper index is
> incredibly simple (in all cases except one) - just pick the lowest
> unused index.
>
> The one exception is USB2 controllers because multiple controllers in
> the same group have the same index. For these we look to see if 1) the
> most recently added USB controller is also a USB2 controller, and 2)
> the group *that* controller belongs to doesn't yet have a controller
> of the exact model we're just now adding - if both are true, the new
> controller gets the same index, but in all other cases we just assign
> the lowest unused index.
>
> With this patch in place and combined with the automatic PCI address
> assignment, we can define a PCIe switch with several ports like this:
>
> <controller type='pci' model='pcie-root-port'/>
> <controller type='pci' model='pcie-switch-upstream-port'/>
> <controller type='pci' model='pcie-switch-downstream-port'/>
> <controller type='pci' model='pcie-switch-downstream-port'/>
> <controller type='pci' model='pcie-switch-downstream-port'/>
> <controller type='pci' model='pcie-switch-downstream-port'/>
> <controller type='pci' model='pcie-switch-downstream-port'/>
> ...
>
> These will each get a unique index, and PCI addresses that connect
> them together appropriately with no pesky numbers required.
> ---
> docs/schemas/domaincommon.rng | 8 ++--
> src/conf/domain_conf.c | 89 +++++++++++++++++++++++++++++++++++++------
> 2 files changed, 82 insertions(+), 15 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 273715c..6abd771 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1701,9 +1701,11 @@
> </define>
> <define name="controller">
> <element name="controller">
> - <attribute name="index">
> - <ref name="unsignedInt"/>
> - </attribute>
> + <optional>
> + <attribute name="index">
> + <ref name="unsignedInt"/>
> + </attribute>
> + </optional>
> <interleave>
> <optional>
> <ref name="alias"/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 728949b..85ffcac 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7850,6 +7850,7 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr
def,
> static virDomainControllerDefPtr
> virDomainControllerDefParseXML(xmlNodePtr node,
> xmlXPathContextPtr ctxt,
> + virDomainDef const *dom,
> unsigned int flags)
> {
> virDomainControllerDefPtr def = NULL;
> @@ -7890,6 +7891,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> if (!(def = virDomainControllerDefNew(type)))
> goto error;
>
> + model = virXMLPropString(node, "model");
> + if (model) {
> + if ((def->model = virDomainControllerModelTypeFromString(def, model))
< 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown model type '%s'"), model);
> + goto error;
> + }
> + }
> +
> idx = virXMLPropString(node, "index");
> if (idx) {
> if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 ||
> @@ -7898,15 +7908,70 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> _("Cannot parse controller index %s"), idx);
> goto error;
> }
> - }
> + } else {
> + /* When no index is provided, find one by looking at what
> + * indexes are already used for this controller type in the
> + * domain.
> + */
> + virDomainControllerDefPtr prev = NULL;
> +
> + if (IS_USB2_CONTROLLER(def)) {
> + /* Attempt to put new USB2 controller at the same index as
> + * other USB2 controllers, but only if this appears to be
> + * the intent. To prove intent: The last USB controller
> + * already on the list must also be a USB2 controller, and
> + * there must not yet be a controller with the exact same
> + * model of this one and the same index as the previously
> + * added controller (e.g., if this controller is a UHCI1,
> + * then the previous controller must be an EHCI1 or a
> + * UHCI[23], and there must not already be a UHCI1
> + * controller with the same index as the previous
> + * controller). If all of these are satisfied, set this
> + * controller to the same index as the previous
> + * controller. NB: we are of course assuming that this
> + * new controller is about to be appended to the domain's
> + * controller list.
> + */
> + int prevIdx;
> + size_t i;
>
> - model = virXMLPropString(node, "model");
> - if (model) {
> - if ((def->model = virDomainControllerModelTypeFromString(def, model))
< 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Unknown model type '%s'"), model);
> - goto error;
> + if (dom->ncontrollers) {
> + prevIdx = dom->ncontrollers - 1;
> + while (prevIdx >= 0 &&
> + dom->controllers[prevIdx]->type !=
VIR_DOMAIN_CONTROLLER_TYPE_USB)
> + prevIdx--;
> + if (prevIdx >= 0)
> + prev = dom->controllers[prevIdx];
> + }
> + /* if the last USB controller isn't USB2, that breaks
> + * the chain, so we need a new index for this new
> + * controller
> + */
> + if (!IS_USB2_CONTROLLER(prev))
> + prev = NULL;
> +
> + /* if prev != NULL, we've found a potential index to
> + * use. Now we need to make sure that index isn't
> + * already used by an existing USB2 controller of the
> + * same model as this new one.
> + */
> + for (i = 0; prev && i < dom->ncontrollers; i++) {
> + if (dom->controllers[i]->type ==
VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> + dom->controllers[i]->idx == prev->idx &&
> + dom->controllers[i]->model == def->model) {
> + /* already have a controller of this model
> + * with the proposed index, so we need to move
> + * to a new index
> + */
> + prev = NULL;
> + }
> + }
> + if (prev)
> + def->idx = prev->idx;
> }
> + /* if none of the above applied, prev will be NULL */
> + if (!prev)
> + def->idx = virDomainControllerFindUnusedIndex(dom, def->type);
> }
>
> cur = node->children;
> @@ -12958,7 +13023,7 @@ virDomainDeviceDefParse(const char *xmlStr,
> break;
> case VIR_DOMAIN_DEVICE_CONTROLLER:
> if (!(dev->data.controller = virDomainControllerDefParseXML(node, ctxt,
> - flags)))
> + def, flags)))
> goto error;
> break;
> case VIR_DOMAIN_DEVICE_GRAPHICS:
> @@ -16157,10 +16222,10 @@ virDomainDefParseXML(xmlDocPtr xml,
> goto error;
>
> for (i = 0; i < n; i++) {
> - virDomainControllerDefPtr controller =
virDomainControllerDefParseXML(nodes[i],
> - ctxt,
> -
flags);
> - if (!controller)
> + virDomainControllerDefPtr controller;
> +
> + if (!(controller = virDomainControllerDefParseXML(nodes[i], ctxt,
> + def, flags)))
> goto error;
>
> /* sanitize handling of "none" usb controller */
>
I agree with the goal of the patch, but I think all the index assignment code
should be moved to somewhere in the PostParse call path. The fact that the
controller ParseXML now needs to act on the entire domain def is a giveaway
that it's in the wrong place.
Also this seems like it should have test suite changes too
Also, formatdomain.html.in has a reference to 'index' being mandatory
- Cole
1:30 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/14/2016 10:56 AM, Cole Robinson wrote:
I agree with the goal of the patch, but I think all the index
assignment code
should be moved to somewhere in the PostParse call path. The fact that the
controller ParseXML now needs to act on the entire domain def is a giveaway
that it's in the wrong place.
I originally did it that way, but there was some problem with it, either
actual or imagined/potential. I *think* possibly the problem was that
auto-added controllers (which is done in the driver-specific postparse,
called prior to the common postparse) are added when an existing
controller of the desired index can't be found, but if an index was
added in postparse, I would want it to be done in the common postparse
(since it is a requirement for *all* drivers); also is the potential
problem that PCI controller indexes must be in place in order for the
PCI address auto-assignment to work, and you had previously expressed a
desire to move that up into one of the postparse functions rather than
having it be a separate function that must be independently called - if
that happened, it would also be done in the driver-specific postparse.
I'll look at it some more and see if I can find an acceptable way to
make everyone happy.
Also this seems like it should have test suite changes too
Yeah, I've been trying to be better about that, but I forgot this time -
some tests that have index-less controllers as the input and indexes in
the output would be useful.
1:38 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/15/2016 02:30 PM, Laine Stump wrote:
On 05/14/2016 10:56 AM, Cole Robinson wrote:
> I agree with the goal of the patch, but I think all the index assignment code
> should be moved to somewhere in the PostParse call path. The fact that the
> controller ParseXML now needs to act on the entire domain def is a giveaway
> that it's in the wrong place.
I originally did it that way, but there was some problem with it, either
actual or imagined/potential. I *think* possibly the problem was that
auto-added controllers (which is done in the driver-specific postparse, called
prior to the common postparse) are added when an existing controller of the
desired index can't be found, but if an index was added in postparse, I would
want it to be done in the common postparse (since it is a requirement for
*all* drivers);
Hmm. Most implicit controllers are added in common code actually, however
there are some added in qemu code it seems, for PCI, which is probably what
you were referring to. In that case we may need to do two passes of index
assignment, or when adding a PCI controller outside common code we just
informally require the hv driver to assign the index themselves.
Unfortunately these types of ordering problems are basically unavoidable in
certain cases... there's no one size fits all ordering for validation, setting
defaults, adding default devices, and assigning addresses.
also is the potential problem that PCI controller indexes must
be in place in order for the PCI address auto-assignment to work, and
you had
previously expressed a desire to move that up into one of the postparse
functions rather than having it be a separate function that must be
independently called - if that happened, it would also be done in the
driver-specific postparse.
I posted patches yesterday adding address assignment to PostParse, but it
comes at the very end of the common PostParse usage, to match how
qemuDomainAssignAddresses is presently used. So at least that bit shouldn't
interfere with index assignment in generic code
- Cole
1:57 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/15/2016 02:38 PM, Cole Robinson wrote:
On 05/15/2016 02:30 PM, Laine Stump wrote:
> On 05/14/2016 10:56 AM, Cole Robinson wrote:
>> I agree with the goal of the patch, but I think all the index assignment code
>> should be moved to somewhere in the PostParse call path. The fact that the
>> controller ParseXML now needs to act on the entire domain def is a giveaway
>> that it's in the wrong place.
> I originally did it that way, but there was some problem with it, either
> actual or imagined/potential. I *think* possibly the problem was that
> auto-added controllers (which is done in the driver-specific postparse, called
> prior to the common postparse) are added when an existing controller of the
> desired index can't be found, but if an index was added in postparse, I would
> want it to be done in the common postparse (since it is a requirement for
> *all* drivers);
Hmm. Most implicit controllers are added in common code actually, however
there are some added in qemu code it seems, for PCI, which is probably what
you were referring to.
There are several controllers/devices added which are not just
hypervisor specific, but specific to particular machinetypes/arches
within that hypervisor. In particular, qemuDomainDefAddDefaultDevices.
In that case we may need to do two passes of index
assignment, or when adding a PCI controller outside common code we just
informally require the hv driver to assign the index themselves.
It's not that the new controller needs an index assigned. It's that the
controller is "maybe added" depending on whether of not there is already
a controller of the requested type at a particular index (usually 0).
For example, when we add a default USB controller in
qemuDomainDefAddDefaultDevices().
As for doing two passes, where would the first pass be run? I don't want
it to be done in the driver-specific postparse because then it would
need to be called separately for every driver, which is prone to people
not doing it. And the common postparse is too late to do any good. The
only place we're left with, other than in the controller parser itself,
is the top-level domain parser function prior to calling the
driver-specific postparse.
Also there is the case where only a single controller is parsed - the
toplevel domain parse is never called there in the first place (of
course I'm skeptical that is ever called for any controller - are any
controllers hotpluggable?)
Unfortunately these types of ordering problems are basically
unavoidable in
certain cases... there's no one size fits all ordering for validation, setting
defaults, adding default devices, and assigning addresses.
Yeah, there's no way to deal with this in the current postparse callback
framework, and the only way to solve all possible ordering problems in
that way would be, it seems, to call all of the callbacks repeatedly in
a loop until it went through an entire cycle without any change :-P
> also is the potential problem that PCI controller indexes must
> be in place in order for the PCI address auto-assignment to work, and you had
> previously expressed a desire to move that up into one of the postparse
> functions rather than having it be a separate function that must be
> independently called - if that happened, it would also be done in the
> driver-specific postparse.
I posted patches yesterday adding address assignment to PostParse, but it
comes at the very end of the common PostParse usage, to match how
qemuDomainAssignAddresses is presently used.
Interesting, I need to look at that. Considering that the way addresses
are assigned could change based on the machinetype/arch, how can you do
that in the common postparse (which doesn't know about things like qemu
capabilities)?
So at least that bit shouldn't
interfere with index assignment in generic code
5:26 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/15/2016 02:57 PM, Laine Stump wrote:
On 05/15/2016 02:38 PM, Cole Robinson wrote:
> On 05/15/2016 02:30 PM, Laine Stump wrote:
>> On 05/14/2016 10:56 AM, Cole Robinson wrote:
>>> I agree with the goal of the patch, but I think all the index assignment
code
>>> should be moved to somewhere in the PostParse call path. The fact that the
>>> controller ParseXML now needs to act on the entire domain def is a giveaway
>>> that it's in the wrong place.
>> I originally did it that way, but there was some problem with it, either
>> actual or imagined/potential. I *think* possibly the problem was that
>> auto-added controllers (which is done in the driver-specific postparse, called
>> prior to the common postparse) are added when an existing controller of the
>> desired index can't be found, but if an index was added in postparse, I
would
>> want it to be done in the common postparse (since it is a requirement for
>> *all* drivers);
> Hmm. Most implicit controllers are added in common code actually, however
> there are some added in qemu code it seems, for PCI, which is probably what
> you were referring to.
There are several controllers/devices added which are not just hypervisor
specific, but specific to particular machinetypes/arches within that
hypervisor. In particular, qemuDomainDefAddDefaultDevices.
> In that case we may need to do two passes of index
> assignment, or when adding a PCI controller outside common code we just
> informally require the hv driver to assign the index themselves.
It's not that the new controller needs an index assigned. It's that the
controller is "maybe added" depending on whether of not there is already a
controller of the requested type at a particular index (usually 0). For
example, when we add a default USB controller in
qemuDomainDefAddDefaultDevices().
Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
called as part of virDomainDefPostParse, via the driver specific
qemuDomainDefPostParse
As for doing two passes, where would the first pass be run? I
don't want it to
be done in the driver-specific postparse because then it would need to be
called separately for every driver, which is prone to people not doing it. And
the common postparse is too late to do any good. The only place we're left
with, other than in the controller parser itself, is the top-level domain
parser function prior to calling the driver-specific postparse.
So the current flow is
virDomainDefPostParse
1) qemuDomainDefPostParse
2) per device qemuDomainDeviceDefPostParse
3) per device virDomainDeviceDefPostParseInternal
4) virDomainDefAddImplicitDevices
<later>
5) qemuDomainAssignAddresses
For qemu, it looks like controllers can be added in step 1, 4, and 5. At the
very least I suspect we need a pass after step #4 since ImplicitControllers
can be added. That may cover everything that your original approach covers,
since all those places aren't hitting the XML parser anyways and instead
building controllers manually AFAICT.
Also there is the case where only a single controller is parsed - the
toplevel
domain parse is never called there in the first place (of course I'm skeptical
that is ever called for any controller - are any controllers hotpluggable?)
> Unfortunately these types of ordering problems are basically unavoidable in
> certain cases... there's no one size fits all ordering for validation, setting
> defaults, adding default devices, and assigning addresses.
Yeah, there's no way to deal with this in the current postparse callback
framework, and the only way to solve all possible ordering problems in that
way would be, it seems, to call all of the callbacks repeatedly in a loop
until it went through an entire cycle without any change :-P
Right. I don't think we need to come up with a 100% future proof solution at
this point, just one that works for all cases we presently care about.
>
> > also is the potential problem that PCI controller indexes must
>> be in place in order for the PCI address auto-assignment to work, and you had
>> previously expressed a desire to move that up into one of the postparse
>> functions rather than having it be a separate function that must be
>> independently called - if that happened, it would also be done in the
>> driver-specific postparse.
> I posted patches yesterday adding address assignment to PostParse, but it
> comes at the very end of the common PostParse usage, to match how
> qemuDomainAssignAddresses is presently used.
Interesting, I need to look at that. Considering that the way addresses are
assigned could change based on the machinetype/arch, how can you do that in
the common postparse (which doesn't know about things like qemu capabilities)?
The driver specific PostParse callbacks can take an opaque value, which is
where we get qemuCaps for example. See
src/qemu/qemu_domain.c:qemuDomainDefPostParse
- Cole
7:39 p.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/16/2016 06:26 PM, Cole Robinson wrote:
On 05/15/2016 02:57 PM, Laine Stump wrote:
> On 05/15/2016 02:38 PM, Cole Robinson wrote:
>> On 05/15/2016 02:30 PM, Laine Stump wrote:
>>> On 05/14/2016 10:56 AM, Cole Robinson wrote:
>>>> I agree with the goal of the patch, but I think all the index assignment
code
>>>> should be moved to somewhere in the PostParse call path. The fact that
the
>>>> controller ParseXML now needs to act on the entire domain def is a
giveaway
>>>> that it's in the wrong place.
>>> I originally did it that way, but there was some problem with it, either
>>> actual or imagined/potential. I *think* possibly the problem was that
>>> auto-added controllers (which is done in the driver-specific postparse,
called
>>> prior to the common postparse) are added when an existing controller of the
>>> desired index can't be found, but if an index was added in postparse, I
would
>>> want it to be done in the common postparse (since it is a requirement for
>>> *all* drivers);
>> Hmm. Most implicit controllers are added in common code actually, however
>> there are some added in qemu code it seems, for PCI, which is probably what
>> you were referring to.
> There are several controllers/devices added which are not just hypervisor
> specific, but specific to particular machinetypes/arches within that
> hypervisor. In particular, qemuDomainDefAddDefaultDevices.
>
>> In that case we may need to do two passes of index
>> assignment, or when adding a PCI controller outside common code we just
>> informally require the hv driver to assign the index themselves.
> It's not that the new controller needs an index assigned. It's that the
> controller is "maybe added" depending on whether of not there is already a
> controller of the requested type at a particular index (usually 0). For
> example, when we add a default USB controller in
> qemuDomainDefAddDefaultDevices().
>
Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
called as part of virDomainDefPostParse, via the driver specific
qemuDomainDefPostParse
Exactly - the driver-specific post-parse is called before the common
post-parse, and the controller indexes need to be known by the time
those controllers are added, because they are added at *specific
indexes* depending on whether or not there is already a controller of
that exact type/index. The auto-indexing is the same for all drivers
though, so if it goes in a post-parse callback it should go in the
common postparse, which happens too late.
(note that we don't need to worry about auto-indexing controllers added
by post-parse functions - they are already using the same method to give
each controller an index immediately).
> As for doing two passes, where would the first pass be run? I
don't want it to
> be done in the driver-specific postparse because then it would need to be
> called separately for every driver, which is prone to people not doing it. And
> the common postparse is too late to do any good. The only place we're left
> with, other than in the controller parser itself, is the top-level domain
> parser function prior to calling the driver-specific postparse.
>
So the current flow is
virDomainDefPostParse
1) qemuDomainDefPostParse
2) per device qemuDomainDeviceDefPostParse
3) per device virDomainDeviceDefPostParseInternal
4) virDomainDefAddImplicitDevices
<later>
5) qemuDomainAssignAddresses
For qemu, it looks like controllers can be added in step 1, 4, and 5. At the
very least I suspect we need a pass after step #4 since ImplicitControllers
can be added. That may cover everything that your original approach covers,
since all those places aren't hitting the XML parser anyways and instead
building controllers manually AFAICT.
The only controllers that need this auto-index step are those added to
the config by the user. None of the controllers added later need it
(because they all determine their own index).
> Also there is the case where only a single controller is parsed -
the toplevel
> domain parse is never called there in the first place (of course I'm skeptical
> that is ever called for any controller - are any controllers hotpluggable?)
>
>
>> Unfortunately these types of ordering problems are basically unavoidable in
>> certain cases... there's no one size fits all ordering for validation,
setting
>> defaults, adding default devices, and assigning addresses.
> Yeah, there's no way to deal with this in the current postparse callback
> framework, and the only way to solve all possible ordering problems in that
> way would be, it seems, to call all of the callbacks repeatedly in a loop
> until it went through an entire cycle without any change :-P
>
Right. I don't think we need to come up with a 100% future proof solution at
this point, just one that works for all cases we presently care about.
Yeah, that was just generalizing on the point that even the current set
of callbacks is insufficient to solve this, and if we add more
callbacks, we'll eventually end up with another problem that all *those*
callbacks can't solve.
If you put too much stuff into callback functions, at some point you end
up with code that is associated with the same functionality but split up
into different unrelated parts of the code, that it becomes impossible
for a newcomer to navigate.
>>> also is the potential problem that PCI controller indexes must
>>> be in place in order for the PCI address auto-assignment to work, and you
had
>>> previously expressed a desire to move that up into one of the postparse
>>> functions rather than having it be a separate function that must be
>>> independently called - if that happened, it would also be done in the
>>> driver-specific postparse.
>> I posted patches yesterday adding address assignment to PostParse, but it
>> comes at the very end of the common PostParse usage, to match how
>> qemuDomainAssignAddresses is presently used.
> Interesting, I need to look at that. Considering that the way addresses are
> assigned could change based on the machinetype/arch, how can you do that in
> the common postparse (which doesn't know about things like qemu capabilities)?
>
The driver specific PostParse callbacks can take an opaque value, which is
where we get qemuCaps for example. See
src/qemu/qemu_domain.c:qemuDomainDefPostParse
I wasn't asking how you could get the qemuCaps into a qemu-specific
callback function. I had thought that you were calling a common
callback for all hypervisors. I wasn't thinking clearly though - of
course it would be different for each hypervisor.
10:18 a.m.
New subject: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
On 05/19/2016 08:39 PM, Laine Stump wrote:
On 05/16/2016 06:26 PM, Cole Robinson wrote:
> On 05/15/2016 02:57 PM, Laine Stump wrote:
>> On 05/15/2016 02:38 PM, Cole Robinson wrote:
>>> On 05/15/2016 02:30 PM, Laine Stump wrote:
>>>> On 05/14/2016 10:56 AM, Cole Robinson wrote:
>>>>> I agree with the goal of the patch, but I think all the index
assignment
>>>>> code
>>>>> should be moved to somewhere in the PostParse call path. The fact
that the
>>>>> controller ParseXML now needs to act on the entire domain def is a
giveaway
>>>>> that it's in the wrong place.
>>>> I originally did it that way, but there was some problem with it, either
>>>> actual or imagined/potential. I *think* possibly the problem was that
>>>> auto-added controllers (which is done in the driver-specific postparse,
>>>> called
>>>> prior to the common postparse) are added when an existing controller of
the
>>>> desired index can't be found, but if an index was added in postparse,
I
>>>> would
>>>> want it to be done in the common postparse (since it is a requirement
for
>>>> *all* drivers);
>>> Hmm. Most implicit controllers are added in common code actually, however
>>> there are some added in qemu code it seems, for PCI, which is probably what
>>> you were referring to.
>> There are several controllers/devices added which are not just hypervisor
>> specific, but specific to particular machinetypes/arches within that
>> hypervisor. In particular, qemuDomainDefAddDefaultDevices.
>>
>>> In that case we may need to do two passes of index
>>> assignment, or when adding a PCI controller outside common code we just
>>> informally require the hv driver to assign the index themselves.
>> It's not that the new controller needs an index assigned. It's that the
>> controller is "maybe added" depending on whether of not there is
already a
>> controller of the requested type at a particular index (usually 0). For
>> example, when we add a default USB controller in
>> qemuDomainDefAddDefaultDevices().
>>
> Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
> called as part of virDomainDefPostParse, via the driver specific
> qemuDomainDefPostParse
Exactly - the driver-specific post-parse is called before the common
post-parse, and the controller indexes need to be known by the time those
controllers are added, because they are added at *specific indexes* depending
on whether or not there is already a controller of that exact type/index. The
auto-indexing is the same for all drivers though, so if it goes in a
post-parse callback it should go in the common postparse, which happens too late.
(note that we don't need to worry about auto-indexing controllers added by
post-parse functions - they are already using the same method to give each
controller an index immediately).
Okay, sorry for the confusion. Still though, putting it inline with the XML
parsing is not a good idea.
>> As for doing two passes, where would the first pass be run? I don't want it
to
>> be done in the driver-specific postparse because then it would need to be
>> called separately for every driver, which is prone to people not doing it. And
>> the common postparse is too late to do any good. The only place we're left
>> with, other than in the controller parser itself, is the top-level domain
>> parser function prior to calling the driver-specific postparse.
>>
> So the current flow is
>
> virDomainDefPostParse
> 1) qemuDomainDefPostParse
> 2) per device qemuDomainDeviceDefPostParse
> 3) per device virDomainDeviceDefPostParseInternal
> 4) virDomainDefAddImplicitDevices
> <later>
> 5) qemuDomainAssignAddresses
>
> For qemu, it looks like controllers can be added in step 1, 4, and 5. At the
> very least I suspect we need a pass after step #4 since ImplicitControllers
> can be added. That may cover everything that your original approach covers,
> since all those places aren't hitting the XML parser anyways and instead
> building controllers manually AFAICT.
The only controllers that need this auto-index step are those added to the
config by the user. None of the controllers added later need it (because they
all determine their own index).
Ah okay, that makes it simpler then IMO. Stick the index assignment code
before the hvspecific/qemuDomainDefPostParse call in virDomainDefPostParse.
Yeah the ordering will look weird, but we can cram stuff in PostParse, test
that it works, and formalize things or come up with a better ordering later,
once we have something testable and working. Mixing things into the XML parser
is when stuff becomes really hard to decouple
>> Also there is the case where only a single controller is parsed - the toplevel
>> domain parse is never called there in the first place (of course I'm
skeptical
>> that is ever called for any controller - are any controllers hotpluggable?)
>>
>>
>>> Unfortunately these types of ordering problems are basically unavoidable in
>>> certain cases... there's no one size fits all ordering for validation,
>>> setting
>>> defaults, adding default devices, and assigning addresses.
>> Yeah, there's no way to deal with this in the current postparse callback
>> framework, and the only way to solve all possible ordering problems in that
>> way would be, it seems, to call all of the callbacks repeatedly in a loop
>> until it went through an entire cycle without any change :-P
>>
> Right. I don't think we need to come up with a 100% future proof solution at
> this point, just one that works for all cases we presently care about.
Yeah, that was just generalizing on the point that even the current set of
callbacks is insufficient to solve this, and if we add more callbacks, we'll
eventually end up with another problem that all *those* callbacks can't solve.
If you put too much stuff into callback functions, at some point you end up
with code that is associated with the same functionality but split up into
different unrelated parts of the code, that it becomes impossible for a
newcomer to navigate.
Yes the web of postparse checks is confusing at the moment. But a big part of
the confusion is because we have things like validation, setting defaults,
assigning address, add adding default devices all sprinkled informally across
multiple different places like generic postparse, hv postparse, XML building,
cli building, opencoded, because we never had a clear plan. Peter's patches
for formalizing a validation callback is a step in the right direction but it
will get worse before it gets better, like with any major code transition.
But what I absolutely think is part of the solution is making the XML parser
be strictly about serializing the XML into a virDomainDef, with as little side
effects as possible. There's already a multitude of little issues stemming
from validation checks or default setting in the XML parser that manual
virDomainDef builders can't leverage. Maybe no other drivers need the index
assignment stuff, dunno, but having it separate from the XML parser is
basically the only way they will be able to consume this. And putting it in
generic PostParse means those drivers get it automatically.
- Cole
3109
days inactive
3118
days old
13 comments
2 participants
participants (2)
-
Cole Robinson -
Laine Stump