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

[Hongwei Bi]

2013/10/7 Daniel P. Berrange <berrange at redhat.com>:
> 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 :|


I agree with that new error code should be added. So the v2 of the
patch has been post.