[libvirt] [PATCH v2] vmx: Adapt to emptyBackingString for cdrom-image
https://bugzilla.redhat.com/show_bug.cgi?id=1266088 We are missing this value for cdrom-image device. Honestly, I have no idea whether it can happen for other disk devices too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- diff to v1: -extened support for scsi CDROM too -added formatting code -added couple of tests src/vmx/vmx.c | 38 +++++++++++++++++++++----- tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx | 5 ++++ tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml | 23 ++++++++++++++++ tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx | 6 ++++ tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml | 23 ++++++++++++++++ tests/vmx2xmltest.c | 3 ++ tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx | 13 +++++++++ tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml | 13 +++++++++ tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx | 14 ++++++++++ tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml | 13 +++++++++ tests/xml2vmxtest.c | 2 ++ 11 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d1cdad3..10fec74 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2246,6 +2246,16 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con * call to this function to parse a CDROM device may handle it. */ goto ignore; + } else if (STREQ(fileName, "emptyBackingString")) { + if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Expecting VMX entry '%s' to be 'cdrom-image' " + "but found '%s'"), deviceType_name, deviceType); + goto cleanup; + } + + virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); + ignore_value(virDomainDiskSetSource(*def, NULL)); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid or not yet handled value '%s' " @@ -2319,6 +2329,16 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con */ goto ignore; } + } else if (STREQ(fileName, "emptyBackingString")) { + if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Expecting VMX entry '%s' to be 'cdrom-image' " + "but found '%s'"), deviceType_name, deviceType); + goto cleanup; + } + + virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); + ignore_value(virDomainDiskSetSource(*def, NULL)); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid or not yet handled value '%s' " @@ -3526,15 +3546,19 @@ virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, if (type == VIR_STORAGE_TYPE_FILE) { const char *src = virDomainDiskGetSource(def); - if (src && ! virFileHasSuffix(src, fileExt)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Image file for %s %s '%s' has " - "unsupported suffix, expecting '%s'"), - busType, deviceType, def->dst, fileExt); + if (src) { + if (!virFileHasSuffix(src, fileExt)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Image file for %s %s '%s' has " + "unsupported suffix, expecting '%s'"), + busType, deviceType, def->dst, fileExt); return -1; - } + } - fileName = ctx->formatFileName(src, ctx->opaque); + fileName = ctx->formatFileName(src, ctx->opaque); + } else { + ignore_value(VIR_STRDUP(fileName, "emptyBackingString")); + } if (fileName == NULL) return -1; diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx new file mode 100644 index 0000000..62fdb3d --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx @@ -0,0 +1,5 @@ +config.version = "8" +virtualHW.version = "4" +ide0:0.present = "true" +ide0:0.deviceType = "cdrom-image" +ide0:0.fileName = "emptyBackingString" diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml new file mode 100644 index 0000000..232200b --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.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='file' device='disk'> + <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'/> + </video> + </devices> +</domain> diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx new file mode 100644 index 0000000..3c6036a --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx @@ -0,0 +1,6 @@ +config.version = "8" +virtualHW.version = "4" +scsi0.present = "true" +scsi0:0.present = "true" +scsi0:0.deviceType = "cdrom-image" +scsi0:0.fileName = "emptyBackingString" diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml new file mode 100644 index 0000000..f2e4c75 --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.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='file' device='disk'> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0'/> + <video> + <model type='vmvga' vram='4096'/> + </video> + </devices> +</domain> diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 0bbf055..a22af75 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -219,14 +219,17 @@ mymain(void) DO_TEST("harddisk-transient", "harddisk-transient"); DO_TEST("cdrom-scsi-file", "cdrom-scsi-file"); + DO_TEST("cdrom-scsi-empty", "cdrom-scsi-empty"); DO_TEST("cdrom-scsi-device", "cdrom-scsi-device"); DO_TEST("cdrom-scsi-raw-device", "cdrom-scsi-raw-device"); DO_TEST("cdrom-scsi-raw-auto-detect", "cdrom-scsi-raw-auto-detect"); 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-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"); + DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect"); DO_TEST("floppy-file", "floppy-file"); DO_TEST("floppy-device", "floppy-device"); diff --git a/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx b/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx new file mode 100644 index 0000000..45c7950 --- /dev/null +++ b/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx @@ -0,0 +1,13 @@ +.encoding = "UTF-8" +config.version = "8" +virtualHW.version = "4" +guestOS = "other" +uuid.bios = "56 4d 9b ef ac d9 b4 e0-c8 f0 ae a8 b9 10 35 15" +displayName = "cdrom-ide-file" +memsize = "4" +numvcpus = "1" +ide0:0.present = "true" +ide0:0.deviceType = "cdrom-image" +ide0:0.fileName = "emptyBackingString" +floppy0.present = "false" +floppy1.present = "false" diff --git a/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml b/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml new file mode 100644 index 0000000..219603e --- /dev/null +++ b/tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml @@ -0,0 +1,13 @@ +<domain type='vmware'> + <name>cdrom-ide-file</name> + <uuid>564d9bef-acd9-b4e0-c8f0-aea8b9103515</uuid> + <memory unit='KiB'>4096</memory> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file' device='cdrom'> + <target dev='hda' bus='ide'/> + </disk> + </devices> +</domain> diff --git a/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx b/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx new file mode 100644 index 0000000..1097cb1 --- /dev/null +++ b/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx @@ -0,0 +1,14 @@ +.encoding = "UTF-8" +config.version = "8" +virtualHW.version = "4" +guestOS = "other" +uuid.bios = "56 4d 9b ef ac d9 b4 e0-c8 f0 ae a8 b9 10 35 15" +displayName = "cdrom-scsi-empty" +memsize = "4" +numvcpus = "1" +scsi0.present = "true" +scsi0:0.present = "true" +scsi0:0.deviceType = "cdrom-image" +scsi0:0.fileName = "emptyBackingString" +floppy0.present = "false" +floppy1.present = "false" diff --git a/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml b/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml new file mode 100644 index 0000000..a5a6d80 --- /dev/null +++ b/tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml @@ -0,0 +1,13 @@ +<domain type='vmware'> + <name>cdrom-scsi-empty</name> + <uuid>564d9bef-acd9-b4e0-c8f0-aea8b9103515</uuid> + <memory unit='KiB'>4096</memory> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file' device='cdrom'> + <target dev='sda' bus='scsi'/> + </disk> + </devices> +</domain> diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index 32bad5f..d970240 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -235,11 +235,13 @@ mymain(void) DO_TEST("harddisk-ide-file", "harddisk-ide-file", 4); DO_TEST("cdrom-scsi-file", "cdrom-scsi-file", 4); + DO_TEST("cdrom-scsi-empty", "cdrom-scsi-empty", 4); DO_TEST("cdrom-scsi-device", "cdrom-scsi-device", 4); DO_TEST("cdrom-scsi-raw-device", "cdrom-scsi-raw-device", 4); DO_TEST("cdrom-scsi-raw-auto-detect", "cdrom-scsi-raw-auto-detect", 4); DO_TEST("cdrom-scsi-passthru", "cdrom-scsi-passthru", 4); DO_TEST("cdrom-ide-file", "cdrom-ide-file", 4); + DO_TEST("cdrom-ide-empty", "cdrom-ide-empty", 4); DO_TEST("cdrom-ide-device", "cdrom-ide-device", 4); DO_TEST("cdrom-ide-raw-device", "cdrom-ide-raw-device", 4); DO_TEST("cdrom-ide-raw-auto-detect", "cdrom-ide-raw-auto-detect", 4); -- 2.4.10
2016-01-20 17:10 GMT+01:00 Michal Privoznik <mprivozn@redhat.com>:
https://bugzilla.redhat.com/show_bug.cgi?id=1266088
We are missing this value for cdrom-image device. Honestly, I have no idea whether it can happen for other disk devices too.
I tried to figure out what emptyBackingString is about, when it might be used and stuff. But I couldn't find anything about it. The only thing I could find were some in-the-wild VMX files that used emptyBackingString as the filename for a client-device backed CDROM device. But client-devices are a special kind of beast as their data doesn't come from a file or a local physical device but is send to the VM from what ever vSphere client app the user happens to use. But this is something libvirt will probably never support, anyway. So dealing with emptyBackingString for cdrom-image devices only is probably good enough for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
diff to v1: -extened support for scsi CDROM too -added formatting code -added couple of tests
src/vmx/vmx.c | 38 +++++++++++++++++++++----- tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx | 5 ++++ tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml | 23 ++++++++++++++++ tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx | 6 ++++ tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml | 23 ++++++++++++++++ tests/vmx2xmltest.c | 3 ++ tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx | 13 +++++++++ tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml | 13 +++++++++ tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx | 14 ++++++++++ tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml | 13 +++++++++ tests/xml2vmxtest.c | 2 ++ 11 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx create mode 100644 tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.vmx create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-ide-empty.xml create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.vmx create mode 100644 tests/xml2vmxdata/xml2vmx-cdrom-scsi-empty.xml
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d1cdad3..10fec74 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2246,6 +2246,16 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con * call to this function to parse a CDROM device may handle it. */ goto ignore; + } else if (STREQ(fileName, "emptyBackingString")) { + if (deviceType && STRCASENEQ(deviceType, "cdrom-image")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Expecting VMX entry '%s' to be 'cdrom-image' " + "but found '%s'"), deviceType_name, deviceType); + goto cleanup; + } + + virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); + ignore_value(virDomainDiskSetSource(*def, NULL)); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid or not yet handled value '%s' "
This is in an if block with this condition if (device == VIR_DOMAIN_DISK_DEVICE_DISK) There is no point in dealing with cdrom-image related stuff in a harddisk section. Just drop this hunk from the patch. Also this hunk results in wrong VMX to XML formatting. See broken test cases below. There is a big if case before this hunk that catches several CDROM related case. it also needs to handle the case STREQ(fileName, "emptyBackingString") to ignore it for this call of the parse function.
@@ -3526,15 +3546,19 @@ virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, if (type == VIR_STORAGE_TYPE_FILE) { const char *src = virDomainDiskGetSource(def);
- if (src && ! virFileHasSuffix(src, fileExt)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Image file for %s %s '%s' has " - "unsupported suffix, expecting '%s'"), - busType, deviceType, def->dst, fileExt); + if (src) { + if (!virFileHasSuffix(src, fileExt)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Image file for %s %s '%s' has " + "unsupported suffix, expecting '%s'"), + busType, deviceType, def->dst, fileExt); return -1; - } + }
- fileName = ctx->formatFileName(src, ctx->opaque); + fileName = ctx->formatFileName(src, ctx->opaque); + } else { + ignore_value(VIR_STRDUP(fileName, "emptyBackingString")); + }
if (fileName == NULL) return -1;
You made the parse function accept emptyBackingString for cdrom-image only. But here you output emptyBackingString for every file backed device, nut just cdrom-image. The new else branch needs to check for this condition: def->device == VIR_DOMAIN_DISK_DEVICE_CDROM
diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx new file mode 100644 index 0000000..62fdb3d --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.vmx @@ -0,0 +1,5 @@ +config.version = "8" +virtualHW.version = "4" +ide0:0.present = "true" +ide0:0.deviceType = "cdrom-image" +ide0:0.fileName = "emptyBackingString" diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml new file mode 100644 index 0000000..232200b --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.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='file' device='disk'>
The VMX file specified a CDROM device, but the XML shows it as a harddisk.
+ <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'/> + </video> + </devices> +</domain> diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx new file mode 100644 index 0000000..3c6036a --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.vmx @@ -0,0 +1,6 @@ +config.version = "8" +virtualHW.version = "4" +scsi0.present = "true" +scsi0:0.present = "true" +scsi0:0.deviceType = "cdrom-image" +scsi0:0.fileName = "emptyBackingString" diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml new file mode 100644 index 0000000..f2e4c75 --- /dev/null +++ b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.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='file' device='disk'>
Again, the VMX file specified a CDROM device, but the XML shows it as a harddisk. This comes from the logic error in the parse function mention above.
+ <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0'/> + <video> + <model type='vmvga' vram='4096'/> + </video> + </devices> +</domain>
ACK, with those things fixed. I attached a diff to be applied on top of this patch with the suggested changes. -- Matthias Bolte http://photron.blogspot.com
participants (2)
-
Matthias Bolte -
Michal Privoznik