[PATCH] vmx: make 'fileName' optional for CD-ROMs
Richard W.M. Jones
rjones at redhat.com
Tue Mar 17 17:09:04 UTC 2020
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 at redhat.com>
> ---
> 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 at redhat.com>
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 at redhat.com>
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
More information about the libvir-list
mailing list