Re: [PATCH] conf: Report sensible error for invalid disk name

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]