virstorageobj: Make virStoragePoolObjAddVol() report an error on failure

Adding a storage volume into a pool is done by calling
virStoragePoolObjAddVol(). This function may fail if another
volume already exists with the same key/name/target. In some
cases the storage driver does check for duplicates before calling
the function. But in some cases (e.g. when refreshing an RBD pool
in virStorageBackendRBDRefreshPool()) it doesn't.

The problem here is that the function reports no error upon
failure and leaves it as an exercise for caller. Well, no caller
does that.

Therefore, make the function report an error. The advantage of
this approach is - the function can report more accurate error
message than any caller ever could.

NB¸ this stems from a discussion on the users list [1], and while
this does NOT solve the original issue, it fixes one of the
symptoms.

1: https://lists.libvirt.org/archives/list/users@lists.libvirt.org/message/BALVNCRQM4KBKGV4RQ7BMKSX7UIJKLQH/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

src/conf/virstorageobj.c

@@ -621,19 +621,44 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj,
     virStorageVolObj *volobj = NULL;
     virStorageVolObjList *volumes = obj->volumes;
 
-    virObjectRWLockWrite(volumes);
-
-    if (!voldef->key || !voldef->name || !voldef->target.path ||
-        g_hash_table_contains(volumes->objsKey, voldef->key) ||
-        g_hash_table_contains(volumes->objsName, voldef->name) ||
-        g_hash_table_contains(volumes->objsPath, voldef->target.path)) {
-        virObjectRWUnlock(volumes);
+    if (!voldef->key) {
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("volume is missing key"));
+        return -1;
+    }
+    if (!voldef->name) {
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("volume is missing name"));
+        return -1;
+    }
+    if (!voldef->target.path) {
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("volume is missing target path"));
         return -1;
     }
 
+    virObjectRWLockWrite(volumes);
+
+    if (g_hash_table_contains(volumes->objsKey, voldef->key)) {
+        virReportError(VIR_ERR_OPERATION_FAILED,
+                       _("volume with key '%1$s' already exist"),
+                       voldef->key);
+        goto error;
+    }
+    if (g_hash_table_contains(volumes->objsName, voldef->name)) {
+        virReportError(VIR_ERR_STORAGE_VOL_EXIST,
+                       _("'%1$s'"), voldef->name);
+        goto error;
+    }
+    if (g_hash_table_contains(volumes->objsPath, voldef->target.path)) {
+        virReportError(VIR_ERR_OPERATION_FAILED,
+                       _("volume with target path '%1$s' already exist"),
+                       voldef->target.path);
+        goto error;
+    }
+
     if (!(volobj = virStorageVolObjNew())) {
-        virObjectRWUnlock(volumes);
-        return -1;
+        goto error;
     }
 
     VIR_WITH_OBJECT_LOCK_GUARD(volobj) {
@@ -652,6 +677,10 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj,
     virObjectUnref(volobj);
     virObjectRWUnlock(volumes);
     return 0;
+
+ error:
+    virObjectRWUnlock(volumes);
+    return -1;
 }