On 02/12/2014 07:29 AM, Michael Chapman wrote:
On Thu, 16 Jan 2014, Peter Krempa wrote:
> On 01/09/14 23:40, Eric Blake wrote:
>> On 01/06/2014 09:44 AM, Peter Krempa wrote:
>>> Separate the steps to create libvirt's volume metadata from the actual
>>> volume building process. This is already done for regular file based
>>> pools to allow job support for storage APIs.
>>> ---
>>> src/storage/storage_backend_logical.c | 60
>>> +++++++++++++++++++++--------------
>>> 1 file changed, 37 insertions(+), 23 deletions(-)
>>>
>>
>> ACK; I'm borderline whether this should wait for the release, though.
>>
>
> Now that 1.2.2 is out I've pushed this one and the rest with the same
> complaint.
Hi Peter,
This breaks volume creation in an LVM pool:
# cat volume.xml
<volume type='block'>
<name>test</name>
<capacity unit='bytes'>10737418240</capacity>
<allocation unit='bytes'>10737418240</allocation>
</volume>
# virsh vol-create vg volume.xml
error: Failed to create vol from volume.xml
error: key in virGetStorageVol must not be NULL
The problem is a storage backend's createVol function is expected to set an
appropriate key, but for an LVM volume this isn't done now until buildVol is
called. The LV's UUID is used as the volume's key.
For the disk backend, volume keys are also generated in buildVol after the
volume is created.
IMO we should revert commits:
commit af1fb38f55d4fb87e0fcaee1e973fa9c6713b1e6
storage: lvm: Separate creating of the volume from building
commit 67ccf91bf29488783bd1fda46b362450f71a2078
storage: disk: Separate creating of the volume from building
unless we have a really good reason not to.
vol-clone and vol-create-from are even worse: libvirtd crashes since the NULL
return from virGetStorageVol isn't handled in storageVolCreateXMLFrom. We
probably need:
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1828,6 +1828,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
pool->volumes.objs[pool->volumes.count++] = newvol;
volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
newvol->key, NULL, NULL);
+ if (!volobj) {
+ pool->volumes.count--;
+ goto cleanup;
+ }
/* Drop the pool lock during volume allocation */
pool->asyncjobs++;
ACK to this patch. No matter what way we fix the actual bug, virGetStorageVol
can still return NULL when we're out of memory. Would you like to write a
commit message and send it as a formal patch, or shall I do it?
Jan