This is perfectly valid in VMWare and the VM just boots with an empty drive.  We
used to just skip the whole drive before, but since we changed how we parse
empty cdrom drives this now results in an error and the user not being able to
even dump the XML.  Instead of erroring out, just keep the drive empty.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/vmx/vmx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index b86dbe9ca267..40e4ef962992 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
                 goto cleanup;
             }
 
+            tmp = ctx->parseFileName(fileName, ctx->opaque);
             virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
-            if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
-                goto cleanup;
-            virDomainDiskSetSource(*def, tmp);
+            /* It is easily possible to have a cdrom with non-existing filename
+             * as the image and vmware just provides an empty cdrom.
+             *
+             * See: https://bugzilla.redhat.com/1903953
+             */
+            if (tmp) {
+                virDomainDiskSetSource(*def, tmp);
+            } else {
+                virResetLastError();
+            }
             VIR_FREE(tmp);
         } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
             virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
-- 
2.29.2

On a Monday in 2020, Martin Kletzander wrote:
>This is perfectly valid in VMWare and the VM just boots with an empty drive.  We
>used to just skip the whole drive before, but since we changed how we parse
>empty cdrom drives this now results in an error and the user not being able to
>even dump the XML.  Instead of erroring out, just keep the drive empty.
>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/vmx/vmx.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
>diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>index b86dbe9ca267..40e4ef962992 100644
>--- a/src/vmx/vmx.c
>+++ b/src/vmx/vmx.c
>@@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
>                 goto cleanup;
>             }
>
>+            tmp = ctx->parseFileName(fileName, ctx->opaque);
>             virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
>-            if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
>-                goto cleanup;
>-            virDomainDiskSetSource(*def, tmp);
>+            /* It is easily possible to have a cdrom with non-existing filename
>+             * as the image and vmware just provides an empty cdrom.
>+             *
>+             * See: https://bugzilla.redhat.com/1903953
>+             */
>+            if (tmp) {
>+                virDomainDiskSetSource(*def, tmp);
>+            } else {
>+                virResetLastError();

Using virResetLastError elsewhere than beginning of a libvirt API is
suspicious. If it's not an error, we should not have logged it in the
first place.

Jano

>+            }
>             VIR_FREE(tmp);
>         } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) {
>             virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
>-- 
>2.29.2
>

On Mon, 2020-12-21 at 12:48 +0100, Martin Kletzander wrote:
> This is perfectly valid in VMWare and the VM just boots with an empty
> drive.  We
> used to just skip the whole drive before, but since we changed how we
> parse
> empty cdrom drives this now results in an error and the user not
> being able to
> even dump the XML.  Instead of erroring out, just keep the drive
> empty.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1903953
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>

Reviewed-by: Tim Wiederhake <twiederh@redhat.com>

> ---
>  src/vmx/vmx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index b86dbe9ca267..40e4ef962992 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2447,10 +2447,18 @@ virVMXParseDisk(virVMXContext *ctx,
> virDomainXMLOptionPtr xmlopt, virConfPtr con
>                  goto cleanup;
>              }
>  
> +            tmp = ctx->parseFileName(fileName, ctx->opaque);
>              virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
> -            if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
> -                goto cleanup;
> -            virDomainDiskSetSource(*def, tmp);
> +            /* It is easily possible to have a cdrom with non-
> existing filename
> +             * as the image and vmware just provides an empty cdrom.
> +             *
> +             * See: https://bugzilla.redhat.com/1903953
> +             */
> +            if (tmp) {
> +                virDomainDiskSetSource(*def, tmp);
> +            } else {
> +                virResetLastError();
> +            }
>              VIR_FREE(tmp);
>          } else if (deviceType && STRCASEEQ(deviceType, "atapi-
> cdrom")) {
>              virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);