[libvirt] [PATCH v2 1/2] storage: Don't update volume objs list before we successfully create one

Martin Kletzander mkletzan at redhat.com
Tue Jun 2 12:44:59 UTC 2015


On Mon, Jun 01, 2015 at 03:14:24PM +0200, Martin Kletzander wrote:
>On Thu, May 28, 2015 at 05:29:54PM +0200, Erik Skultety wrote:
>>We do update pool volume object list before we actually create any
>>volume. If buildVol fails, we then try to delete the volume in the
>>storage as well as remove it from our structures. The problem is, that
>>any backend that supports both buildVol and deleteVol would fail in this
>>case which is completely unnecessary. This patch causes the update to
>>take place after we know a volume has been created successfully, thus no
>>removal in case of a buildVol failure is necessary.
>>
>>https://bugzilla.redhat.com/show_bug.cgi?id=1223177
>>---
>>src/storage/storage_driver.c | 31 +++++++++++++------------------
>>1 file changed, 13 insertions(+), 18 deletions(-)
>>
>>diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>>index ac4a74a..25a9656 100644
>>--- a/src/storage/storage_driver.c
>>+++ b/src/storage/storage_driver.c
>>@@ -1,7 +1,7 @@
>>/*
>> * storage_driver.c: core driver for storage APIs
>> *
>>- * Copyright (C) 2006-2014 Red Hat, Inc.
>>+ * Copyright (C) 2006-2015 Red Hat, Inc.
>> * Copyright (C) 2006-2008 Daniel P. Berrange
>> *
>> * This library is free software; you can redistribute it and/or
>>@@ -1812,9 +1812,6 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>        goto cleanup;
>>    }
>>
>>-    if (VIR_REALLOC_N(pool->volumes.objs,
>>-                      pool->volumes.count+1) < 0)
>>-        goto cleanup;
>>
>>    if (!backend->createVol) {
>>        virReportError(VIR_ERR_NO_SUPPORT,
>>@@ -1832,14 +1829,6 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>    if (backend->createVol(obj->conn, pool, voldef) < 0)
>>        goto cleanup;
>>
>>-    pool->volumes.objs[pool->volumes.count++] = voldef;
>>-    volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
>>-                              voldef->key, NULL, NULL);
>>-    if (!volobj) {
>>-        pool->volumes.count--;
>>-        goto cleanup;
>>-    }
>>-
>>    if (VIR_ALLOC(buildvoldef) < 0) {
>>        voldef = NULL;
>>        goto cleanup;
>>@@ -1868,14 +1857,20 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>        voldef->building = false;
>>        pool->asyncjobs--;
>>
>>-        if (buildret < 0) {
>>-            VIR_FREE(buildvoldef);
>>-            storageVolDeleteInternal(volobj, backend, pool, voldef,
>>-                                     0, false);
>>-            voldef = NULL;
>>+        if (buildret < 0)
>>            goto cleanup;
>>-        }
>>+    }
>>+
>>+    if (VIR_REALLOC_N(pool->volumes.objs,
>>+                      pool->volumes.count+1) < 0)
>>+        goto cleanup;
>>
>>+    pool->volumes.objs[pool->volumes.count++] = voldef;
>>+    volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
>>+                              voldef->key, NULL, NULL);
>
>I know it's pre-existing, but if you switch these two lines, you don't
>have to do this unnecessary complicated cleanup (i.e. two lines) here.
>
>>+    if (!volobj) {
>>+        pool->volumes.count--;
>>+        goto cleanup;
>
>Also, if this fails, you need to delete the the volume anyway,
>otherwise you have data inconsistency again.  How about doing this:
>

Actually, you don't.  My head just gave up on my here.  But it would
be nice to keep the definition in the list and not decrement the count
there with 'pool->volumes.count--'.  We would return OOM error, but
apart from that everything would be consistent.

So ACK with that one line removed.

> VIR_REALLOC_N(pool->volumes.objs, ...);
> virGetStorageVol(...);
> buildVol();
>
>This way the last thing that can fail is the one you don't want to
>revert, but because it is the last one already, you don't need to
>revert it (adding the definition to the list and increasing the count
>can't fail).
>
>>    }
>>
>>    if (backend->refreshVol &&
>>--
>>1.9.3
>>
>>--
>>libvir-list mailing list
>>libvir-list at redhat.com
>>https://www.redhat.com/mailman/listinfo/libvir-list



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150602/5e7a12f2/attachment-0001.sig>


More information about the libvir-list mailing list