On 09/07/2017 01:00 PM, Andrea Bolognani wrote:
> virDomainDefAddController() is very convenient, but it is only passed
> a limited number of information about the controller.
>
> When adding USB controllers, knowing whether the controller is a
> master or a companion is important because it can influence the order
> devices show up on the QEMU command line, and ultimately whether the
> guest can be started at all.
After reading this, I was expecting that virDomainDefAddController() was
going to get new args. But what we really got was a cleanup of
virDomainDefAddController() that doesn't change its behavior, and a
modification to virDomainDevAddUSBController() that avoids calling
virDomainDefAddController() when it's adding a USB2 controller. Nothing
wrong with that, I was just surprised :-)
BTW, did you figure out a way that a bug could be experienced without
this change, or is this all theoretical?
>
> To make sure USB controllers will always be sorted correctly,
> allocate the virDomainControllerDef structure explicitly, fill in the
> relevant information manually and call virDomainControllerInsert()
> directly.
Maybe you could spell out more explicitly that you're replacing calls to
virDomainDefAddController() with [blah blah], so it's easier for dense
people like me to pick up on what's happening.
>
> Clean up both virDomainDefAddUSBController() itself and
> virDomainDefAddController() while at it.
The cleanup of virDomainDefAddController seems unrelated to the change
to virDomainDevAddUSBController() (since it is eliminating calls to
virDomainDefAddController()). I'm debating whether or not it's worth
asking you to split those two things into separate patches - that would
have made it quicker for me to figure out what was being done, but
that's all water under the bridge now, so it would just be creating work
for you for no real gain.
> The behavior is not altered,
> as evidenced by the lack of updates to the test suite.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 61 +++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2fc1fc340..b1b9d161c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16837,7 +16837,7 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
> virDomainControllerDefPtr cont;
>
> if (!(cont = virDomainControllerDefNew(type)))
> - return NULL;
> + goto error;
>
> if (idx < 0)
> idx = virDomainControllerFindUnusedIndex(def, type);
> @@ -16845,12 +16845,14 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
> cont->idx = idx;
> cont->model = model;
>
> - if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0) {
> - VIR_FREE(cont);
> - return NULL;
> - }
> + if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0)
> + goto error;
>
> return cont;
> +
> + error:
> + virDomainControllerDefFree(cont);
> + return NULL;
> }
As said previously - the bit above doesn't fit with the rest of the patch.
>
>
> @@ -16870,15 +16872,14 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
> int
> virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model)
> {
> - virDomainControllerDefPtr cont; /* this is a *copy* of the DefPtr */
> + virDomainControllerDefPtr cont;
Why did you remove the comment? It's just a (poorly worded) reminder
that cont shouldn't be directly free'd, since it's only a copy of the
pointer that was already added to def->controllers.
>
> - cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> - idx, model);
> - if (!cont)
> - return -1;
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, model)))
> + goto error;
>
> if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1)
> - return 0;
> + goto done;
>
> /* When the initial controller is ich9-usb-ehci, also add the
> * companion controllers
> @@ -16886,25 +16887,45 @@ virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model)
>
> idx = cont->idx; /* in case original request was "-1" */
>
> - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1)))
> - return -1;
> + if (!(cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB)))
> + goto error;
> +
> + cont->idx = idx;
> + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1;
> cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> cont->info.master.usb.startport = 0;
>
> - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2)))
> - return -1;
> + if (virDomainControllerInsert(def, cont) < 0)
> + goto error;
> +
> + if (!(cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB)))
> + goto error;
> +
> + cont->idx = idx;
> + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2;
> cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> cont->info.master.usb.startport = 2;
>
> - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3)))
> - return -1;
> + if (virDomainControllerInsert(def, cont) < 0)
> + goto error;
> +
> + if (!(cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB)))
> + goto error;
> +
> + cont->idx = idx;
> + cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3;
> cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> cont->info.master.usb.startport = 4;
>
> + if (virDomainControllerInsert(def, cont) < 0)
> + goto error;
> +
> + done:
> return 0;
> +
> + error:
> + virDomainControllerDefFree(cont);
> + return -1;
> }
>
>
It all looks fine other than the unrelated stuff (which is also fine,
just a bit out of place). ACK. I'll leave it to you whether to split the
patch or push it as it is.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
