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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list