[libvirt] [PATCH] storage: use valid XML for awkward volume names

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131121/38f9b846/attachment-0001.sig>