On 08/20/2013 05:08 AM, Osier Yang wrote:

Introduced by commit e0139e30444:

1777 /* Updating pool metadata */

(40) Event var_deref_op: Dereferencing null pointer "newvol". Also see events: [assign_zero]

1778 pool->def->allocation += newvol->allocation; 1779 pool->def->available -= newvol->allocation; --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7908ba6..63a954b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1758,7 +1758,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,

origvol->building = 0; newvol->building = 0; - newvol = NULL; pool->asyncjobs--;

if (origpool) {

... The next condition is: if (buildret < 0) { virStoragePoolObjUnlock(pool); storageVolDelete(volobj, 0); pool = NULL; goto cleanup; } Since previously we'd have 'newvol = NULL;' already, there would need to be one added here too.. Since, prior to this there's code: pool->volumes.objs[pool->volumes.count++] = newvol; which saves the pointer... Perhaps it'd work better to do the following: unsigned long long allocation = 0x0ULL; ... allocation = newvol->allocation; newvol = NULL; ... pool->def->allocation += allocation; pool->def->available -= allocation;

@@ -1781,6 +1780,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; + newvol = NULL;

and this would become unnecessary

volobj = NULL;

cleanup:

On 08/20/2013 05:08 AM, Osier Yang wrote:

Introduced by commit e0139e30444. virStorageVolDefFree free'ed the pointers that are still used by the added volume object, this changes it back to VIR_FREE. --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 63a954b..883e4e9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1618,7 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); - virStorageVolDefFree(buildvoldef); + VIR_FREE(buildvoldef); if (pool) virStoragePoolObjUnlock(pool); return ret;

Perhaps a comment "/* Free just the shallow copy of buildvoldef */". Just for the clarity of why you wouldn't want to use virStorageVolDefFree(). Of course the same possible other solution applies here as well as it did in your 1/2 patch where you define a local allocation variable to handle the pool->def->{allocation|available} math. That way buildvoldef moves back inside the "if (backend->buildVol)"... ACK either way though. John