[libvirt PATCH] vmx: Don't error out on missing filename for cdrom
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, Dec 21, 2020 at 01:09:13PM +0100, Ján Tomko wrote:
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.
Yeah, I did not like that either, but esx, you know... Since it's you, asking co nicely, I'll give it another shot.
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);
participants (3)
-
Ján Tomko -
Martin Kletzander -
Tim Wiederhake