Re: [libvirt] [PATCH 4/9] Allow cloning volumes with no capacity specified

On Thu, Feb 19, 2015 at 15:59:12 +0100, Ján Tomko wrote: > We can get the capacity from the input volume. I had to trace the code to understand what this patch was about ...

> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index a67d50c..dd33436 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -1055,7 +1055,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, > if (convert) > virCommandAddArg(cmd, inputPath); > virCommandAddArg(cmd, vol->target.path); > - if (!convert) > + if (!convert && size_arg) Note that this change is on the "!convert" (create) path ... [1] > virCommandAddArgFormat(cmd, "%lluK", size_arg); > > return cmd; > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index bc16e87..409b486 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -1810,7 +1810,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, > goto cleanup; > } > > - newvol = virStorageVolDefParseString(pool->def, xmldesc, 0); > + newvol = virStorageVolDefParseString(pool->def, xmldesc, > + VIR_VOL_XML_PARSE_NO_CAPACITY); > if (newvol == NULL) > goto cleanup; Few lines below this change there's the following hunk: /* Is there ever a valid case for this? */ if (newvol->target.capacity < origvol->target.capacity) newvol->target.capacity = origvol->target.capacity; Which sets the capacity to the capacity of the original volume. While this is probably semantically OK it might be worth tweaking the comment.

> > diff --git a/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv > new file mode 100644 > index 0000000..9073b1b > --- /dev/null > +++ b/tests/storagevolxml2argvdata/qcow2-nocapacity-convert-prealloc.argv > @@ -0,0 +1,4 @@ > +qemu-img convert -f raw -O qcow2 \ [1] ... while the test you are adding is testing the convert path. I think that the hunk [1] belongs to a different patch as the size argument was added only on the "create" path anyways. > +-o encryption=on,preallocation=metadata \ > +/var/lib/libvirt/images/sparse.img \ > +/var/lib/libvirt/images/OtherDemo.img ACK after the release with hunk [1] removed.