[libvirt] [PATCH v2] fix a ambiguous output of the command:'virsh vol-create-as'
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..fd14237 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_VOL_EXISTS = 90, /* the storage vol already exists */ } virErrorNumber; /** diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6c39284..d419a36 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1538,8 +1538,8 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; if (virStorageVolDefFindByName(pool, voldef->name)) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("storage vol '%s' already exists"), voldef->name); + virReportError(VIR_ERR_STORAGE_VOL_EXISTS, + _("'%s'"), voldef->name); goto cleanup; } diff --git a/src/util/virerror.c b/src/util/virerror.c index ca25678..3f55cec 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1004,6 +1004,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Storage volume not found: %s"); break; + case VIR_ERR_STORAGE_VOL_EXISTS: + if (info == NULL) + errmsg = _("this storage volume exists already"); + else + errmsg = _("storage volume %s exists already"); + break; case VIR_ERR_STORAGE_PROBE_FAILED: if (info == NULL) errmsg = _("Storage pool probe failed"); -- 1.7.1
On 07.10.2013 16:04, 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..fd14237 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_VOL_EXISTS = 90, /* the storage vol already exists */ } virErrorNumber;
/** diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6c39284..d419a36 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1538,8 +1538,8 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup;
if (virStorageVolDefFindByName(pool, voldef->name)) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("storage vol '%s' already exists"), voldef->name); + virReportError(VIR_ERR_STORAGE_VOL_EXISTS, + _("'%s'"), voldef->name); goto cleanup; }
diff --git a/src/util/virerror.c b/src/util/virerror.c index ca25678..3f55cec 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1004,6 +1004,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Storage volume not found: %s"); break; + case VIR_ERR_STORAGE_VOL_EXISTS: + if (info == NULL) + errmsg = _("this storage volume exists already"); + else + errmsg = _("storage volume %s exists already"); + break; case VIR_ERR_STORAGE_PROBE_FAILED: if (info == NULL) errmsg = _("Storage pool probe failed");
ACKed and pushed. Michal
On 10/07/2013 11:03 AM, Michal Privoznik wrote:
On 07.10.2013 16:04, 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..fd14237 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_VOL_EXISTS = 90, /* the storage vol already exists */
Indentation is odd. Furthermore, existing practice is VIR_ERR_DOM_EXIST and VIR_ERR_NETWORK_EXIST; we should use the singular form here. Please, let's fix this up before a release bakes in the inconsistent naming. Also, your subject line isn't grammatically correct (should have been 'an ambiguous') - but that's too late to change now.
ACKed and pushed.
While at it, should we add VIR_ERR_*_EXIST for all the other libvirt objects that can cause problems when trying to create a new object of an already-existing name? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
2013/10/8 Eric Blake <eblake@redhat.com>:
On 10/07/2013 11:03 AM, Michal Privoznik wrote:
On 07.10.2013 16:04, 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..fd14237 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_VOL_EXISTS = 90, /* the storage vol already exists */
Indentation is odd. Furthermore, existing practice is VIR_ERR_DOM_EXIST and VIR_ERR_NETWORK_EXIST; we should use the singular form here. Please, let's fix this up before a release bakes in the inconsistent naming.
Also, your subject line isn't grammatically correct (should have been 'an ambiguous') - but that's too late to change now.
ACKed and pushed.
While at it, should we add VIR_ERR_*_EXIST for all the other libvirt objects that can cause problems when trying to create a new object of an already-existing name?
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As I understand it, it's necessary to add more error codes to represent specific meanings. And I'm sorry about the grammatically correct. I would avoid it from now on.
2013/10/8 Michal Privoznik <mprivozn@redhat.com>:
On 07.10.2013 16:04, 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..fd14237 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_VOL_EXISTS = 90, /* the storage vol already exists */ } virErrorNumber;
/** diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6c39284..d419a36 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1538,8 +1538,8 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup;
if (virStorageVolDefFindByName(pool, voldef->name)) { - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("storage vol '%s' already exists"), voldef->name); + virReportError(VIR_ERR_STORAGE_VOL_EXISTS, + _("'%s'"), voldef->name); goto cleanup; }
diff --git a/src/util/virerror.c b/src/util/virerror.c index ca25678..3f55cec 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1004,6 +1004,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Storage volume not found: %s"); break; + case VIR_ERR_STORAGE_VOL_EXISTS: + if (info == NULL) + errmsg = _("this storage volume exists already"); + else + errmsg = _("storage volume %s exists already"); + break; case VIR_ERR_STORAGE_PROBE_FAILED: if (info == NULL) errmsg = _("Storage pool probe failed");
ACKed and pushed.
Michal
Pushed; Thanks.
participants (3)
-
Eric Blake -
Hongwei Bi -
Michal Privoznik