On 2011?11?23? 11:09, Wen Ruo Lv wrote:

Redefine pool after pool creation failure,give out err msg for non-exsisted vg when starting pool. ref: http://www.redhat.com/archives/libvir-list/2011-November/msg01152.html http://www.redhat.com/archives/libvir-list/2011-November/msg01152.html

src/storage/storage_backend_logical.c:just give out err msg for non-exsisted vg,not combine pool-build in pool-create ,since undo build for kinds of pool(iscsi,multipath) when err occours cause a lot of trouble.

src/storage/storage_driver.c:add redefinition of pool,pool may change in pool-start,redefine it after free the pool

Signed-off-by: Wen Ruo Lv<lvroyce@linux.vnet.ibm.com> --- src/storage/storage_backend_logical.c | 10 ++++++++++ src/storage/storage_driver.c | 29 +++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3c3e736..994c792 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -50,6 +50,16 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, { const char *cmdargv[4];

+ cmdargv[0] = VGS; + cmdargv[1] = pool->def->source.name; + cmdargv[2] = NULL;

use virCommand APIs once creating new codes. Please see http://libvirt.org/internals/command.html

+ + if (virRun(cmdargv, NULL)< 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("vg '%s' does not exsist,try pool-build"), pool->def->source.name); + return -1; + }

I tend to disagree this, virStorageBackendLogicalSetActive is not only used for startPool, but also stopPool. It's strange to see "try pool-build" while the user wants to stop it.

+ cmdargv[0] = VGCHANGE; cmdargv[1] = on ? "-ay" : "-an"; cmdargv[2] = pool->def->source.name; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8c2d6e1..a5cbafe 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -526,6 +526,7 @@ storagePoolCreate(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + int duppool;

virCheckFlags(0, NULL);

@@ -533,7 +534,7 @@ storagePoolCreate(virConnectPtr conn, if (!(def = virStoragePoolDefParseString(xml))) goto cleanup;

- if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1)< 0) + if ((duppool = virStoragePoolObjIsDuplicate(&driver->pools, def, 1))< 0) goto cleanup;

if (virStoragePoolSourceFindDuplicate(&driver->pools, def)< 0) @@ -544,26 +545,46 @@ storagePoolCreate(virConnectPtr conn,

if (!(pool = virStoragePoolObjAssignDef(&driver->pools, def))) goto cleanup; - def = NULL;

if (backend->startPool&& backend->startPool(conn, pool)< 0) { virStoragePoolObjRemove(&driver->pools, pool); - pool = NULL; + + if (duppool) { + if (!(def = virStoragePoolDefParseString(xml))) + goto cleanup; + + pool = virStoragePoolObjAssignDef(&driver->pools, def); + } + else + pool = NULL;

coding style nits. It should be: } else { pool = NULL; } Please take a look at HACKING before making patches. NACK, the pool is already redefined with previous virStoragePoolObjAssignDef, this is just a duplicate work, and this won't fix your problem, the pool def will be free()ed.

+ + def = NULL; goto cleanup; }

if (backend->refreshPool(conn, pool)< 0) { if (backend->stopPool) backend->stopPool(conn, pool); + virStoragePoolObjRemove(&driver->pools, pool); - pool = NULL; + + if (duppool) { + if (!(def = virStoragePoolDefParseString(xml))) + goto cleanup; + pool = virStoragePoolObjAssignDef(&driver->pools, def); + } + else + pool = NULL; + + def = NULL;

Likewise. Regards, Osier