[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