Re: [libvirt] [PATCH] fix a ambiguous output of the command:'virsh vol-create-as'

On Mon, Oct 07, 2013 at 10:48:30AM +0200, Michal Privoznik wrote: > On 07.10.2013 07:20, Hongwei Bi wrote: > > I created a storage volume(eg: test) from a storage pool(eg:vg10) using > > the following command:"virsh vol-create-as --pool vg10 --name test --capacity 300M." > > When I re-executed the above command, the output was as the following: > > "error: Failed to create vol test > > error: Storage volume not found: storage vol 'test' already exists" > > > > I think the output "Storage volume not found" is not appropriate. Because in fact storage > > vol test has been found at this time. And then I think virErrorNumber should includes > > VIR_ERR_STORAGE_EXIST which can also be used elsewhere. So I make this patch. The result > > is as following: > > "error: Failed to create vol test > > error: storage volume 'test' exists already" > > > > --- > > include/libvirt/virterror.h | 1 + > > src/storage/storage_driver.c | 4 ++-- > > src/util/virerror.c | 6 ++++++ > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > > index c1960c8..28ef30a 100644 > > --- a/include/libvirt/virterror.h > > +++ b/include/libvirt/virterror.h > > @@ -296,6 +296,7 @@ typedef enum { > > VIR_ERR_ACCESS_DENIED = 88, /* operation on the object/resource > > was denied */ > > VIR_ERR_DBUS_SERVICE = 89, /* error from a dbus service */ > > + VIR_ERR_STORAGE_EXIST = 90, /* the storage vol already exist */ I'd suggest this should be VIR_ERR_STORAGE_VOL_EXISTS. > > What about instead of inventing new error code re-use the old VIR_ERR_OPERATION_FAILED? Something like we have in virDomainObjListAddLocked: > > if (STRNEQ(vm->def->name, def->name)) { > virUUIDFormat(vm->def->uuid, uuidstr); > virReportError(VIR_ERR_OPERATION_FAILED, > _("domain '%s' is already defined with uuid %s"), > vm->def->name, uuidstr); > goto error; > } I'd actually suggest that we should add a new error code for domains and other object types too. I think it is useful to be explicit about duplicate / clashing names/ids. Regards, 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 :|