On 08/21/2017 11:41 AM, Ján Tomko wrote:
> When parsing the config, we look for the SCSI controllers one by one,
> remembering their models, then let virDomainDefAddImplicitDevices
> add them if any SCSI disk is using them.
>
> Since these controllers are not really implicit (they are present
> in the source config), add them explicitly.
>
> This patch maintains the behavior of not adding a controller
> if it was present in the config, but no disk was using it.
>
> This also resolves the memory leak of virVMXParseConfig overwriting
> the video device added by calling virDomainDefAddImplicitDevices
> before the parsing is finished.
>
> Reported-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/vmx/vmx.c | 32 ++++++++++++--------------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
>
My compiler isn't happy with this
CC remote/libvirt_driver_remote_la-remote_driver.lo
vmx/vmx.c: In function 'virVMXParseConfig':
vmx/vmx.c:1662:84: error: ordered comparison of pointer with integer
zero [-Werror=extra]
controller,
scsi_virtualDev[controller]) < 0)
^
cc1: all warnings being treated as errors
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 3e2f4c3e1..849cfc6b1 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1650,6 +1650,18 @@ virVMXParseConfig(virVMXContext *ctx,
> if (def->disks[def->ndisks] != NULL)
> ++def->ndisks;
> }
> +
> + }
> +
> + /* add all the SCSI controllers we've seen, up until the last one that is
> + * currently used by a disk */
> + if (def->ndisks != 0) {
> + virDomainDeviceInfoPtr info = &def->disks[def->ndisks - 1]->info;
> + for (controller = 0; controller <= info->addr.drive.controller; controller++) {
I dunno - perhaps feels 'safer' to use 4 instead of
info->addr.drive.controller, but I also see the value of keeping things
as they are... Guess it always looks strange to go from 0 .. <= #...
IDC either way, your call.
> + if (virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
> + controller, scsi_virtualDev[controller]) < 0)
The < 0 is the incorrect comparison, should be:
if (!virDomainDefAddController
Don't forget to properly indent the second line too.
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
> + goto cleanup;
> + }
> }
>
> /* def:disks (ide) */
> @@ -1689,26 +1701,6 @@ virVMXParseConfig(virVMXContext *ctx,
> ++def->ndisks;
> }
>
> - /* def:controllers */
> - if (virDomainDefAddImplicitDevices(def) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add controllers"));
> - goto cleanup;
> - }
> -
> - for (controller = 0; controller < def->ncontrollers; ++controller) {
> - if (def->controllers[controller]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> - if (def->controllers[controller]->idx > 3) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("SCSI controller index %d out of [0..3] range"),
> - def->controllers[controller]->idx);
> - goto cleanup;
> - }
> -
> - def->controllers[controller]->model =
> - scsi_virtualDev[def->controllers[controller]->idx];
> - }
> - }
> -
> /* def:fss */
> if (virVMXGetConfigBoolean(conf, "isolation.tools.hgfs.disable",
> &hgfs_disabled, true, true) < 0) {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
