[libvirt] [PATCH v2] vmx: Adapt to emptyBackingString for cdrom-image
Matthias Bolte
matthias.bolte at googlemail.com
Sun Jan 24 14:44:22 UTC 2016
2016-01-20 17:10 GMT+01:00 Michal Privoznik <mprivozn at 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 at 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
-------------- next part --------------
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 10fec74..4fd0a1d 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2234,6 +2234,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
(*def)->transient = STRCASEEQ(mode,
"independent-nonpersistent");
} else if (virFileHasSuffix(fileName, ".iso") ||
+ STREQ(fileName, "emptyBackingString") ||
(deviceType &&
(STRCASEEQ(deviceType, "atapi-cdrom") ||
STRCASEEQ(deviceType, "cdrom-raw") ||
@@ -2246,16 +2247,6 @@ 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' "
@@ -3556,7 +3547,7 @@ virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def,
}
fileName = ctx->formatFileName(src, ctx->opaque);
- } else {
+ } else if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
ignore_value(VIR_STRDUP(fileName, "emptyBackingString"));
}
diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml
index 232200b..e086379 100644
--- a/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml
+++ b/tests/vmx2xmldata/vmx2xml-cdrom-ide-empty.xml
@@ -11,7 +11,7 @@
<on_reboot>restart</on_reboot>
<on_crash>destroy</on_crash>
<devices>
- <disk type='file' device='disk'>
+ <disk type='file' device='cdrom'>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
diff --git a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml
index f2e4c75..56ad678 100644
--- a/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml
+++ b/tests/vmx2xmldata/vmx2xml-cdrom-scsi-empty.xml
@@ -11,7 +11,7 @@
<on_reboot>restart</on_reboot>
<on_crash>destroy</on_crash>
<devices>
- <disk type='file' device='disk'>
+ <disk type='file' device='cdrom'>
<target dev='sda' bus='scsi'/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
More information about the libvir-list
mailing list