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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 25, 2015 at 10:05:35AM +0100, Peter Krempa wrote:
> 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 ...
> 

I have added a commit message:
    In virStorageVolCreateXML, add VIR_VOL_XML_PARSE_NO_CAPACITY
    to the call parsing the XML of the new volume to make the capacity
    optional.

    If the capacity is omitted, use the capacity of the old volume.
    We already do that for values that are less than the original
    volume capacity.

> > 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.
> 

tweaked the comment:
    /* Use the original volume's capacity in case the new capacity
     * is less than that, or it was omitted */

> >  
> > 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.

and removed the unrelated hunk.

Jan

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]