On 11/21/2013 04:57 PM, Eric Blake wrote:
volume defs during pool refresh and not from xml reparsing; but you do have a point that on CreateVol, we need to be careful to ignore any input key and use the one that the storage volume would have generated normally; likewise, we must ensure we don't leak memory.
I'm still checking...
I verified that the only user XML paths are virStorageVolCreateXML and virStorageVolCreateXMLFrom; and that both paths filter through the storage backend->createVol() before ever attempting to use the key in other locations. So all that remains is auditing each backend's createVol: backend_fs.c always reset key before setting it during createVol; nothing to change. backend_disk.c, on the other hand, enters some code that behaves differently if key is NULL (if so, image is newly created, populate the key) compared to non-NULL (look for matching key - even if user's key was wrong!) so I didn't even bother checking backend_logical, backend_rbd, or backend_sheepdog. Instead:
ACK if you drop this hunk or fix virStorageBackendDiskCreateVol.
I'll reply again with what I squash in after auditing all paths where the user passes in volume XML to make sure we aren't leaking or using the wrong key.
I squashed in this, which avoids the problem for all storage backends. diff --git i/src/storage/storage_driver.c w/src/storage/storage_driver.c index b3f0871..3b4715a 100644 --- i/src/storage/storage_driver.c +++ w/src/storage/storage_driver.c @@ -1554,6 +1554,9 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } + /* Wipe any key the user may have suggested, as volume creation + * will generate the canonical key. */ + VIR_FREE(voldef->key); if (backend->createVol(obj->conn, pool, voldef) < 0) { goto cleanup; } @@ -1729,7 +1732,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, pool->volumes.count+1) < 0) goto cleanup; - /* 'Define' the new volume so we get async progress reporting */ + /* 'Define' the new volume so we get async progress reporting. + * Wipe any key the user may have suggested, as volume creation + * will generate the canonical key. */ + VIR_FREE(newvol->key); if (backend->createVol(obj->conn, pool, newvol) < 0) { goto cleanup; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org