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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
