[libvirt] [PATCH] conf: Report sensible error for invalid disk name
The error "... but the cause is unknown" appeared for XMLs similar to this: <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/dev/zero'/> <target dev='sr0'/> </disk> Notice unsupported disk type (for the driver), but also no address specified. The first part is not a problem and we should not abort immediately because of that, but the combination with the address unknown was causing an unspecified error. --- src/util/util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..d5b2c97 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) { } } - if (!ptr) + if (!ptr) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown disk name '%s' and no address specified"), + name); return -1; + } for (i = 0; *ptr; i++) { idx = (idx + (i < 1 ? 0 : 1)) * 26; -- 1.8.0
On 11/20/12 14:55, Martin Kletzander wrote:
The error "... but the cause is unknown" appeared for XMLs similar to this:
<disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/dev/zero'/> <target dev='sr0'/> </disk>
Notice unsupported disk type (for the driver), but also no address specified. The first part is not a problem and we should not abort immediately because of that, but the combination with the address unknown was causing an unspecified error. --- src/util/util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..d5b2c97 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) { } }
- if (!ptr) + if (!ptr) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown disk name '%s' and no address specified"), + name); return -1; + }
Some of the call paths of this function the callers do their own error reporting (see src/qemu/qemu_command.c) based on the return value. This should be consolidated. In case of this function I'd probably rather go for adding the error message on the appropriate places as the inability to parse the disk ID is ignored on some calls (maybe that should be fixed in the first place and than the error reporting is okay here.)
for (i = 0; *ptr; i++) { idx = (idx + (i < 1 ? 0 : 1)) * 26;
Peter
On 11/20/2012 03:27 PM, Peter Krempa wrote:
On 11/20/12 14:55, Martin Kletzander wrote:
The error "... but the cause is unknown" appeared for XMLs similar to this:
<disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/dev/zero'/> <target dev='sr0'/> </disk>
Notice unsupported disk type (for the driver), but also no address specified. The first part is not a problem and we should not abort immediately because of that, but the combination with the address unknown was causing an unspecified error. --- src/util/util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..d5b2c97 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) { } }
- if (!ptr) + if (!ptr) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown disk name '%s' and no address specified"), + name); return -1; + }
Some of the call paths of this function the callers do their own error reporting (see src/qemu/qemu_command.c) based on the return value. This should be consolidated.
In case of this function I'd probably rather go for adding the error message on the appropriate places as the inability to parse the disk ID is ignored on some calls (maybe that should be fixed in the first place and than the error reporting is okay here.)
You're right, I went through the code just looking for the "un-errored" negative return and haven't thought about that deeply enough. I figured when the error gets set on two places, it'll be simply shadowed. Sending a v2 in a minute. Martin
On Tue, Nov 20, 2012 at 02:55:28PM +0100, Martin Kletzander wrote:
The error "... but the cause is unknown" appeared for XMLs similar to this:
<disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/dev/zero'/> <target dev='sr0'/> </disk>
Notice unsupported disk type (for the driver), but also no address specified. The first part is not a problem and we should not abort immediately because of that, but the combination with the address unknown was causing an unspecified error. --- src/util/util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..d5b2c97 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) { } }
- if (!ptr) + if (!ptr) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown disk name '%s' and no address specified"), + name); return -1; + }
for (i = 0; *ptr; i++) { idx = (idx + (i < 1 ? 0 : 1)) * 26;
IMHO, you should really be adding an ATTRIBUTE_NONNULL annotation and then fixing the callers not to invoke it if the disk name they have is Null. Adding virReportError here is wrong, because a number of callers will already report errors. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/20/2012 03:45 PM, Daniel P. Berrange wrote:
On Tue, Nov 20, 2012 at 02:55:28PM +0100, Martin Kletzander wrote:
The error "... but the cause is unknown" appeared for XMLs similar to this:
<disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/dev/zero'/> <target dev='sr0'/> </disk>
Notice unsupported disk type (for the driver), but also no address specified. The first part is not a problem and we should not abort immediately because of that, but the combination with the address unknown was causing an unspecified error. --- src/util/util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/util.c b/src/util/util.c index 75b18c1..d5b2c97 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2189,8 +2189,12 @@ int virDiskNameToIndex(const char *name) { } }
- if (!ptr) + if (!ptr) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown disk name '%s' and no address specified"), + name); return -1; + }
for (i = 0; *ptr; i++) { idx = (idx + (i < 1 ? 0 : 1)) * 26;
IMHO, you should really be adding an ATTRIBUTE_NONNULL annotation and then fixing the callers not to invoke it if the disk name they have is Null.
The pointer is not the function attribute, but ...
Adding virReportError here is wrong, because a number of callers will already report errors.
... you're right as Peter, v2 is on it's way. Martin
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Peter Krempa