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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 28, 2015 at 05:29:55PM +0200, Erik Skultety wrote:

RBD API returns negative value of errno, in that case we can silently ignore if RBD tries to delete a non-existent volume, just like FS backend does. --- src/storage/storage_backend_rbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ae4bcb3..8e8d7a7 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -1,7 +1,7 @@ /* * storage_backend_rbd.c: storage backend for RBD (RADOS Block Device) handling * - * Copyright (C) 2013-2014 Red Hat, Inc. + * Copyright (C) 2013-2015 Red Hat, Inc. * Copyright (C) 2012 Wido den Hollander

I wonder how come nobody complained in reply to this patch already...

* * This library is free software; you can redistribute it and/or @@ -426,7 +426,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn, VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name, vol->name);

if (flags & VIR_STORAGE_VOL_DELETE_ZEROED) - VIR_WARN("%s", _("This storage backend does not supported zeroed removal of volumes")); + VIR_WARN("%s", _("This storage backend does not support zeroed removal of volumes"));

This could be another trivial patch, but it's *so* trivial, I'm OK with keeping it here.

if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) goto cleanup; @@ -435,7 +435,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn, goto cleanup;

r = rbd_remove(ptr.ioctx, vol->name); - if (r < 0) { + if (r < 0 && (-r) != ENOENT) {

This makes sense. ACK.

virReportSystemError(-r, _("failed to remove volume '%s/%s'"), pool->def->source.name, vol->name); goto cleanup; -- 1.9.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list