Re: [PATCH] vmx: make 'fileName' optional for CD-ROMs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 17, 2020 at 05:25:38PM +0100, Pino Toscano wrote:
> It seems like CD-ROMs may have no 'fileName' property specified in case
> there is nothing configured as attachment for the drive. Hence, make
> sure that virVMXParseDisk() do not consider it mandatory anymore,
> considering it an empty block cdrom device. Sadly virVMXParseDisk() is
> used also to parse disk and floppies, so make sure that a NULL fileName
> is handled in cdrom-related paths.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1808610
> 
> Signed-off-by: Pino Toscano <ptoscano@xxxxxxxxxx>
> ---
>  src/vmx/vmx.c                                 | 22 ++++++++++--------
>  .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx |  4 ++++
>  .../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++++++++++++++++++
>  tests/vmx2xmltest.c                           |  1 +
>  4 files changed, 40 insertions(+), 10 deletions(-)
>  create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
>  create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index f0140129a2..58ae966fd4 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2207,7 +2207,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>          goto cleanup;
>  
>      /* vmx:fileName -> def:src, def:type */
> -    if (virVMXGetConfigString(conf, fileName_name, &fileName, false) < 0)
> +    if (virVMXGetConfigString(conf, fileName_name, &fileName, true) < 0)
>          goto cleanup;
>  
>      /* vmx:writeThrough -> def:cachemode */
> @@ -2218,7 +2218,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>  
>      /* Setup virDomainDiskDef */
>      if (device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> -        if (virStringHasCaseSuffix(fileName, ".vmdk")) {
> +        if (fileName && virStringHasCaseSuffix(fileName, ".vmdk")) {
>              char *tmp;
>  
>              if (deviceType != NULL) {
> @@ -2254,7 +2254,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>              if (mode)
>                  (*def)->transient = STRCASEEQ(mode,
>                                                "independent-nonpersistent");
> -        } else if (virStringHasCaseSuffix(fileName, ".iso") ||
> +        } else if (fileName == NULL ||
> +                   virStringHasCaseSuffix(fileName, ".iso") ||
>                     STREQ(fileName, "emptyBackingString") ||
>                     (deviceType &&
>                      (STRCASEEQ(deviceType, "atapi-cdrom") ||
> @@ -2277,7 +2278,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>              goto cleanup;
>          }
>      } else if (device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> -        if (virStringHasCaseSuffix(fileName, ".iso")) {
> +        if (fileName && virStringHasCaseSuffix(fileName, ".iso")) {
>              char *tmp;
>  
>              if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
> @@ -2295,7 +2296,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>                  goto cleanup;
>              }
>              VIR_FREE(tmp);
> -        } else if (virStringHasCaseSuffix(fileName, ".vmdk")) {
> +        } else if (fileName && virStringHasCaseSuffix(fileName, ".vmdk")) {
>              /*
>               * This function was called in order to parse a CDROM device, but
>               * .vmdk files are for harddisk devices only. Just ignore it,
> @@ -2306,7 +2307,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>          } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
>              virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
>  
> -            if (STRCASEEQ(fileName, "auto detect")) {
> +            if (fileName && STRCASEEQ(fileName, "auto detect")) {
>                  ignore_value(virDomainDiskSetSource(*def, NULL));
>                  (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
>              } else if (virDomainDiskSetSource(*def, fileName) < 0) {
> @@ -2317,7 +2318,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>              (*def)->device = VIR_DOMAIN_DISK_DEVICE_LUN;
>              virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
>  
> -            if (STRCASEEQ(fileName, "auto detect")) {
> +            if (fileName && STRCASEEQ(fileName, "auto detect")) {
>                  ignore_value(virDomainDiskSetSource(*def, NULL));
>                  (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
>              } else if (virDomainDiskSetSource(*def, fileName) < 0) {
> @@ -2325,7 +2326,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>              }
>          } else if (busType == VIR_DOMAIN_DISK_BUS_SCSI &&
>                     deviceType && STRCASEEQ(deviceType, "scsi-passthru")) {
> -            if (STRPREFIX(fileName, "/vmfs/devices/cdrom/")) {
> +            if (fileName && STRPREFIX(fileName, "/vmfs/devices/cdrom/")) {
>                  /* SCSI-passthru CD-ROMs actually are device='lun' */
>                  (*def)->device = VIR_DOMAIN_DISK_DEVICE_LUN;
>                  virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
> @@ -2341,7 +2342,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>                   */
>                  goto ignore;
>              }
> -        } else if (STREQ(fileName, "emptyBackingString")) {
> +        } else if (fileName && STREQ(fileName, "emptyBackingString")) {
>              if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Expecting VMX entry '%s' to be 'cdrom-image' "
> @@ -2355,7 +2356,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Invalid or not yet handled value '%s' "
>                               "for VMX entry '%s' for device type '%s'"),
> -                           fileName, fileName_name,
> +                           fileName ? fileName : "(not present)",
> +                           fileName_name,
>                             deviceType ? deviceType : "unknown");
>              goto cleanup;
>          }
> diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
> new file mode 100644
> index 0000000000..36286cb20f
> --- /dev/null
> +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
> @@ -0,0 +1,4 @@
> +config.version = "8"
> +virtualHW.version = "4"
> +ide0:0.present = "true"
> +ide0:0.deviceType = "atapi-cdrom"
> diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
> new file mode 100644
> index 0000000000..af4a5ff9f6
> --- /dev/null
> +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
> @@ -0,0 +1,23 @@
> +<domain type='vmware'>
> +  <uuid>00000000-0000-0000-0000-000000000000</uuid>
> +  <memory unit='KiB'>32768</memory>
> +  <currentMemory unit='KiB'>32768</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='i686'>hvm</type>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <disk type='block' device='cdrom'>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='ide' index='0'/>
> +    <video>
> +      <model type='vmvga' vram='4096' primary='yes'/>
> +    </video>
> +  </devices>
> +</domain>
> diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c
> index 8d7b8ba2a4..1966aed6fe 100644
> --- a/tests/vmx2xmltest.c
> +++ b/tests/vmx2xmltest.c
> @@ -218,6 +218,7 @@ mymain(void)
>      DO_TEST("cdrom-scsi-passthru", "cdrom-scsi-passthru");
>      DO_TEST("cdrom-ide-file", "cdrom-ide-file");
>      DO_TEST("cdrom-ide-empty", "cdrom-ide-empty");
> +    DO_TEST("cdrom-ide-empty-2", "cdrom-ide-empty-2");
>      DO_TEST("cdrom-ide-device", "cdrom-ide-device");
>      DO_TEST("cdrom-ide-raw-device", "cdrom-ide-raw-device");
>      DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect");

Firstly I can confirm that the patch does work, so:

  Tested-by: Richard W.M. Jones <rjones@xxxxxxxxxx>

My only worry about this patch is that it relies on fileName now
possibly being NULL, which means if there is any case that you've
missed now — or one added in future — which doesn't consider that
fileName might be NULL then it'll crash (libvirtd? or virsh? I'm not
sure).  I wonder if therefore it would be safer to set the string to a
known-good non-NULL value such as ‘strdup ("emptyBackingString")’?

Anyway as far as the patch goes it does seem fine so:

  Reviewed-by: Richard W.M. Jones <rjones@xxxxxxxxxx>

Thanks for looking at this one.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux