4:51 a.m.
v1 is:
https://www.redhat.com/archives/libvir-list/2020-March/msg00616.html
Changes from v1:
- added a patch to shortcut few checks
- use NULLSTR
- handle floppies (poor guys)
Pino Toscano (2):
vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk()
vmx: make 'fileName' optional for CD-ROMs
src/vmx/vmx.c | 67 ++++++++++---------
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 ++
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++++++
tests/vmx2xmltest.c | 1 +
4 files changed, 62 insertions(+), 33 deletions(-)
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
--
2.25.1
4:51 a.m.
New subject: [PATCH v2 1/2] vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk()
Move earlier the checks for skipping a hard disk when parsing a CD-DROM,
and for skipping a CD-ROM when parsing a hard disk. This should have no
behaviour changes, and avoids to add repeated checks in following
commits.
Signed-off-by: Pino Toscano <ptoscano(a)redhat.com>
---
src/vmx/vmx.c | 48 ++++++++++++++++++++++++------------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index f0140129a2..bfc9bc7404 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2218,7 +2218,21 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
/* Setup virDomainDiskDef */
if (device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (virStringHasCaseSuffix(fileName, ".vmdk")) {
+ if (virStringHasCaseSuffix(fileName, ".iso") ||
+ STREQ(fileName, "emptyBackingString") ||
+ (deviceType &&
+ (STRCASEEQ(deviceType, "atapi-cdrom") ||
+ STRCASEEQ(deviceType, "cdrom-raw") ||
+ (STRCASEEQ(deviceType, "scsi-passthru") &&
+ STRPREFIX(fileName, "/vmfs/devices/cdrom/"))))) {
+ /*
+ * This function was called in order to parse a harddisk device,
+ * but .iso files, 'atapi-cdrom', 'cdrom-raw', and
'scsi-passthru'
+ * CDROM devices are for CDROM devices only. Just ignore it, another
+ * call to this function to parse a CDROM device may handle it.
+ */
+ goto ignore;
+ } else if (virStringHasCaseSuffix(fileName, ".vmdk")) {
char *tmp;
if (deviceType != NULL) {
@@ -2254,20 +2268,6 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
if (mode)
(*def)->transient = STRCASEEQ(mode,
"independent-nonpersistent");
- } else if (virStringHasCaseSuffix(fileName, ".iso") ||
- STREQ(fileName, "emptyBackingString") ||
- (deviceType &&
- (STRCASEEQ(deviceType, "atapi-cdrom") ||
- STRCASEEQ(deviceType, "cdrom-raw") ||
- (STRCASEEQ(deviceType, "scsi-passthru") &&
- STRPREFIX(fileName, "/vmfs/devices/cdrom/"))))) {
- /*
- * This function was called in order to parse a harddisk device,
- * but .iso files, 'atapi-cdrom', 'cdrom-raw', and
'scsi-passthru'
- * CDROM devices are for CDROM devices only. Just ignore it, another
- * call to this function to parse a CDROM device may handle it.
- */
- goto ignore;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
@@ -2277,7 +2277,15 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
goto cleanup;
}
} else if (device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
- if (virStringHasCaseSuffix(fileName, ".iso")) {
+ if (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,
+ * another call to this function to parse a harddisk device may
+ * handle it.
+ */
+ goto ignore;
+ } else if (virStringHasCaseSuffix(fileName, ".iso")) {
char *tmp;
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
@@ -2295,14 +2303,6 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
goto cleanup;
}
VIR_FREE(tmp);
- } else if (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,
- * another call to this function to parse a harddisk device may
- * handle it.
- */
- goto ignore;
} else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom"))
{
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
--
2.25.1
4:51 a.m.
New subject: [PATCH v2 2/2] vmx: make 'fileName' optional for CD-ROMs
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- and floppy-related paths.
https://bugzilla.redhat.com/show_bug.cgi?id=1808610
Signed-off-by: Pino Toscano <ptoscano(a)redhat.com>
---
src/vmx/vmx.c | 25 ++++++++++---------
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 +++
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++++++++++++++++
tests/vmx2xmltest.c | 1 +
4 files changed, 41 insertions(+), 12 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 bfc9bc7404..6c6ef7acf3 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,8 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
/* Setup virDomainDiskDef */
if (device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (virStringHasCaseSuffix(fileName, ".iso") ||
+ 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, ".vmdk")) {
+ 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,
@@ -2285,7 +2286,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
* handle it.
*/
goto ignore;
- } else if (virStringHasCaseSuffix(fileName, ".iso")) {
+ } else if (fileName && virStringHasCaseSuffix(fileName,
".iso")) {
char *tmp;
if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) {
@@ -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,7 @@ 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,
+ NULLSTR(fileName), fileName_name,
deviceType ? deviceType : "unknown");
goto cleanup;
}
@@ -2365,10 +2366,10 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
if (virDomainDiskSetSource(*def, fileName) < 0)
goto cleanup;
} else if (fileType != NULL && STRCASEEQ(fileType, "file")) {
- char *tmp;
+ char *tmp = NULL;
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
- if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
+ if (fileName && !(tmp = ctx->parseFileName(fileName,
ctx->opaque)))
goto cleanup;
if (virDomainDiskSetSource(*def, tmp) < 0) {
VIR_FREE(tmp);
@@ -2379,7 +2380,7 @@ 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,
+ NULLSTR(fileName), 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
5:04 a.m.
On a Wednesday in 2020, Pino Toscano wrote:
v1 is:
https://www.redhat.com/archives/libvir-list/2020-March/msg00616.html
Changes from v1:
- added a patch to shortcut few checks
- use NULLSTR
- handle floppies (poor guys)
Pino Toscano (2):
vmx: shortcut earlier few 'ignore' cases in virVMXParseDisk()
vmx: make 'fileName' optional for CD-ROMs
src/vmx/vmx.c | 67 ++++++++++---------
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx | 4 ++
.../vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml | 23 +++++++
tests/vmx2xmltest.c | 1 +
4 files changed, 62 insertions(+), 33 deletions(-)
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.vmx
create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty-2.xml
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
7:32 a.m.
Works, so:
Tested-by: Richard W.M. Jones <rjones(a)redhat.com>
I have no further comments about the patches themselves other than
what I said on the first version.
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
1710
days inactive
1710
days old
4 comments
3 participants
participants (3)
-
Ján Tomko -
Pino Toscano -
Richard W.M. Jones