Re: [libvirt] [PATCH v2] vmx: Adapt to emptyBackingString for cdrom-image

[Matthias Bolte]

2016-01-20 17:10 GMT+01:00 Michal Privoznik :
> 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 
> ---
>
> 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);
> 

[libvirt] [PATCH v2] vmx: Adapt to emptyBackingString for cdrom-image

[Michal Privoznik]

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 
---

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 000..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"