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
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@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");
--
2.25.1
On a Tuesday in 2020, 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@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
>
>@@ -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)",
You can use NULLSTR(fileName) to get a "<null>" in the error message.
Also, there is one more virReportError just like this below
in the FLOPPY section.
>+ fileName_name,
> deviceType ? deviceType : "unknown");
> goto cleanup;
> }
With the other virReportError touched (I don't care which way):
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
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@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@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@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
(Apparently forgot to send it yesterday, so sending it with a small
addedum.)
On Tuesday, 17 March 2020 18:09:04 CET Richard W.M. Jones wrote:
> 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).
In case now (even in v2) fileName is used without checking, it will
crash libvirt, as the esx/vmware drivers are built-in in the library.
> I wonder if therefore it would be safer to set the string to a
> known-good non-NULL value such as ‘strdup ("emptyBackingString")’?
Thought about that, however "emptyBackingString" seems like a magic
identifier, and it would be trading one hack with another, somehow.
--
Pino Toscano
On Wed, Mar 18, 2020 at 01:44:18PM +0100, Pino Toscano wrote:
> (Apparently forgot to send it yesterday, so sending it with a small
> addedum.)
>
> On Tuesday, 17 March 2020 18:09:04 CET Richard W.M. Jones wrote:
> > 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).
>
> In case now (even in v2) fileName is used without checking, it will
> crash libvirt, as the esx/vmware drivers are built-in in the library.
>
> > I wonder if therefore it would be safer to set the string to a
> > known-good non-NULL value such as ‘strdup ("emptyBackingString")’?
>
> Thought about that, however "emptyBackingString" seems like a magic
> identifier, and it would be trading one hack with another, somehow.
Sure, I've no objections if you're happy with it, so you can
add my Reviewed-by tag to v2.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
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@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")) {
Disk without the path does not make sense, if you just error out here then you
don't need to modify each and every line here.
> 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"
Is this happening even without the `startConnected = "FALSE"` ? I thought we
should just make the filename optional if:
1) startConnected = FALSE and
2) the device is a CD-ROM
but if this works for you, then I'm also fine with it I guess, no need to try to
keep the code clean, I also don't like polishing something ancient. Although
this code was written just over 10 years ago, so it's almost new...
(Honestly, kudos to Matthias since his his "First version" still works =D)
Maybe we could use startConnected similarly to what we have with startupPolicy.
But who knows.
>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");
Can't you just do
DO_TEST("cdrom-ide-empty-2", "cdrom-ide-empty");
and save one file?
Anyway, if you want to change it the way I thought of it then fine, if not, then
fine too and:
Reviewed-by-I-guess: Martin Kletzander <mkletzan@redhat.com>
> 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");
>--
>2.25.1
>
© 2016 - 2024 Red Hat, Inc.