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
