[libvirt] [PATCH v2 3/3] virsh: allow metadata preallocation when creating volumes

Peter Krempa pkrempa at redhat.com
Wed Nov 28 13:02:41 UTC 2012


On 11/12/12 17:08, Ján Tomko wrote:
> Add --prealloc-metadata flag to these commands:
> vol-clone
> vol-create
> vol-create-as
> vol-create-from
> ---
>   tools/virsh-volume.c |   25 +++++++++++++++++++++----
>   tools/virsh.pod      |   11 ++++++++---
>   2 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index f219de9..223487e 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -124,6 +124,7 @@ static const vshCmdOptDef opts_vol_create_as[] = {
>        N_("the backing volume if taking a snapshot")},
>       {"backing-vol-format", VSH_OT_STRING, 0,
>        N_("format of backing volume if taking a snapshot")},
> +    {"prealloc-metadata", VSH_OT_BOOL, 0, N_("preallocate metadata")},
>       {NULL, 0, 0, NULL}
>   };
>
> @@ -146,7 +147,10 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>       const char *snapshotStrVol = NULL, *snapshotStrFormat = NULL;
>       unsigned long long capacity, allocation = 0;
>       virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    unsigned long flags = 0;
>
> +    if(vshCommandOptBool(cmd, "prealloc-metadata"))

Missing space after if. Breaks syntax-check.

> +        flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
>       if (!(pool = vshCommandOptPoolBy(ctl, cmd, "pool", NULL,
>                                        VSH_BYNAME)))
>           return false;
> @@ -256,7 +260,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>           goto cleanup;
>       }
>       xml = virBufferContentAndReset(&buf);
> -    vol = virStorageVolCreateXML(pool, xml, 0);
> +    vol = virStorageVolCreateXML(pool, xml, flags);
>       VIR_FREE(xml);
>       virStoragePoolFree(pool);
>
> @@ -287,6 +291,7 @@ static const vshCmdInfo info_vol_create[] = {
>   static const vshCmdOptDef opts_vol_create[] = {
>       {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name")},
>       {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML vol description")},
> +    {"prealloc-metadata", VSH_OT_BOOL, 0, N_("preallocate metadata")},
>       {NULL, 0, 0, NULL}
>   };
>
> @@ -297,8 +302,11 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd)
>       virStorageVolPtr vol;
>       const char *from = NULL;
>       bool ret = true;
> +    unsigned int flags = 0;
>       char *buffer;
>
> +    if(vshCommandOptBool(cmd, "prealloc-metadata"))

here too...

> +        flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
>       if (!(pool = vshCommandOptPoolBy(ctl, cmd, "pool", NULL,
>                                              VSH_BYNAME)))
>           return false;
> @@ -314,7 +322,7 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd)
>           return false;
>       }
>
> -    vol = virStorageVolCreateXML(pool, buffer, 0);
> +    vol = virStorageVolCreateXML(pool, buffer, flags);
>       VIR_FREE(buffer);
>       virStoragePoolFree(pool);
>
> @@ -343,6 +351,7 @@ static const vshCmdOptDef opts_vol_create_from[] = {
>       {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML vol description")},
>       {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("input vol name or key")},
>       {"inputpool", VSH_OT_STRING, 0, N_("pool name or uuid of the input volume's pool")},
> +    {"prealloc-metadata", VSH_OT_BOOL, 0, N_("preallocate metadata")},
>       {NULL, 0, 0, NULL}
>   };
>
> @@ -354,10 +363,13 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd)
>       const char *from = NULL;
>       bool ret = false;
>       char *buffer = NULL;
> +    unsigned int flags;

You are not initializing flags if --prealloc-metadata is not specified

>
>       if (!(pool = vshCommandOptPool(ctl, cmd, "pool", NULL)))
>           goto cleanup;
>
> +    if(vshCommandOptBool(cmd, "prealloc-metadata"))

(missing space)

> +        flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
>       if (vshCommandOptString(cmd, "file", &from) <= 0) {
>           goto cleanup;
>       }
> @@ -370,7 +382,7 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd)
>           goto cleanup;
>       }
>
> -    newvol = virStorageVolCreateXMLFrom(pool, buffer, inputvol, 0);
> +    newvol = virStorageVolCreateXMLFrom(pool, buffer, inputvol, flags);

and calling libvirt API with possible garbage.

>
>       if (newvol != NULL) {
>           vshPrint(ctl, _("Vol %s created from input vol %s\n"),
> @@ -434,6 +446,7 @@ static const vshCmdOptDef opts_vol_clone[] = {
>       {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("orig vol name or key")},
>       {"newname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("clone name")},
>       {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")},
> +    {"prealloc-metadata", VSH_OT_BOOL, 0, N_("preallocate metadata")},
>       {NULL, 0, 0, NULL}
>   };
>
> @@ -446,10 +459,14 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd)
>       char *origxml = NULL;
>       xmlChar *newxml = NULL;
>       bool ret = false;
> +    unsigned int flags;

Uninitialized flags.

>
>       if (!(origvol = vshCommandOptVol(ctl, cmd, "vol", "pool", NULL)))
>           goto cleanup;
>
> +    if(vshCommandOptBool(cmd, "prealloc-metadata"))

missing space.

> +        flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> +
>       origpool = virStoragePoolLookupByVolume(origvol);
>       if (!origpool) {
>           vshError(ctl, "%s", _("failed to get parent pool"));
> @@ -469,7 +486,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd)
>           goto cleanup;
>       }
>
> -    newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, 0);
> +    newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, flags);
>
>       if (newvol != NULL) {
>           vshPrint(ctl, _("Vol %s cloned from %s\n"),
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 0984e6e..f8fa6aa 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2364,13 +2364,14 @@ Returns the UUID of the named I<pool>.
>
>   =over 4
>
> -=item B<vol-create> I<pool-or-uuid> I<FILE>
> +=item B<vol-create> I<pool-or-uuid> I<FILE> [I<--prealloc-metadata>]
>
>   Create a volume from an XML <file>.
>   I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in.
>   I<FILE> is the XML <file> with the volume definition. An easy way to create the
>   XML <file> is to use the B<vol-dumpxml> command to obtain the definition of a
>   pre-existing volume.
> +[I<--prealloc-metadata>] preallocate metadata (for qcow2 images)

I haven't read the other patches so I don't know what this actually 
does. And the documentation here doesn't help much. From the point of 
view of the user (who hasn't read the code) it's the same as me.

Could you please add more docs what this option actually does?

>
>   B<Example>
>
> @@ -2379,7 +2380,7 @@ B<Example>
>    virsh vol-create differentstoragepool newvolume.xml
>
>   =item B<vol-create-from> I<pool-or-uuid> I<FILE> [I<--inputpool>
> -I<pool-or-uuid>] I<vol-name-or-key-or-path>
> +I<pool-or-uuid>] I<vol-name-or-key-or-path> [I<--prealloc-metadata>]
>
>   Create a volume, using another volume as input.
>   I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in.
> @@ -2387,10 +2388,12 @@ I<FILE> is the XML <file> with the volume definition.
>   I<--inputpool> I<pool-or-uuid> is the name or uuid of the storage pool the
>   source volume is in.
>   I<vol-name-or-key-or-path> is the name or key or path of the source volume.
> +[I<--prealloc-metadata>] preallocate metadata (for qcow2 images)
>
>   =item B<vol-create-as> I<pool-or-uuid> I<name> I<capacity>
>   [I<--allocation> I<size>] [I<--format> I<string>] [I<--backing-vol>
>   I<vol-name-or-key-or-path>] [I<--backing-vol-format> I<string>]
> +[I<--prealloc-metadata>]
>
>   Create a volume from a set of arguments.
>   I<pool-or-uuid> is the name or UUID of the storage pool to create the volume
> @@ -2407,9 +2410,10 @@ volume to be used if taking a snapshot of an existing volume.
>   I<--backing-vol-format> I<string> is the format of the snapshot backing volume;
>   raw, bochs, qcow, qcow2, qed, vmdk, host_device. These are, however, meant for
>   file based storage pools.
> +[I<--prealloc-metadata>] preallocate metadata (for qcow2 images)
>
>   =item B<vol-clone> [I<--pool> I<pool-or-uuid>] I<vol-name-or-key-or-path>
> -I<name>
> +I<name> [I<--prealloc-metadata>]
>
>   Clone an existing volume.  Less powerful, but easier to type, version of
>   B<vol-create-from>.
> @@ -2417,6 +2421,7 @@ I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool to create
>   the volume in.
>   I<vol-name-or-key-or-path> is the name or key or path of the source volume.
>   I<name> is the name of the new volume.
> +[I<--prealloc-metadata>] preallocate metadata (for qcow2 images)
>
>   =item B<vol-delete> [I<--pool> I<pool-or-uuid>] I<vol-name-or-key-or-path>
>
>


Peter




More information about the libvir-list mailing list