[libvirt] [PATCH 4/8] storage: lvm: Separate creating of the volume from building

[Ján Tomko]

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

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