[libvirt] [PATCH] Added snapshot backing store parameters to virsh vol-create-as.

[Chris Lalancette]

On 06/06/10 - 07:09:37PM, Justin Clift wrote:
> ---
>  tools/virsh.c   |   52
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  tools/virsh.pod |    7 ++++++-
>  2 files changed, 57 insertions(+), 2 deletions(-)

Hey Justin,
     There are a few issues with this patch stylistically:

1)  First, it didn't apply here; it seems like carriage returns were added
at 80 characters, which made for a corrupt patch.  I know you mentioned you
were going to use git-send-mail for patches; did you use it for this one?

2)  While comments are definitely appreciated, we use the C-style /* */ for
comments instead of C++ style //.

3)  Please wrap lines at 80 columns wherever possible.  You don't have to do
it in the middle of strings, but splitting along commas and other punctuation
makes the patches easier to read.

A few more comments inline...

> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 1279f41..3ba549b 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -5274,6 +5274,8 @@ static const vshCmdOptDef opts_vol_create_as[] = {
>      {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ, N_("size of the vol
> with optional k,M,G,T suffix")},
>      {"allocation", VSH_OT_STRING, 0, N_("initial allocation size
> with optional k,M,G,T suffix")},
>      {"format", VSH_OT_STRING, 0, N_("file format type
> raw,bochs,qcow,qcow2,vmdk")},
> +    {"snapshot-source-vol", VSH_OT_STRING, 0, N_("the source volume
> if taking a snapshot")},
> +    {"snapshot-source-format", VSH_OT_STRING, 0, N_("format of the
> source volume if taking a snapshot")},
>      {NULL, 0, 0, NULL}
>  };
>  @@ -5313,6 +5315,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>      int found;
>      char *xml;
>      char *name, *capacityStr, *allocationStr, *format;
> +    char *snapshotStrVol, *snapshotStrFormat;
>      unsigned long long capacity, allocation = 0;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>  @@ -5339,6 +5342,8 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>          vshError(ctl, _("Malformed size %s"), allocationStr);
>       format = vshCommandOptString(cmd, "format", &found);
> +    snapshotStrVol = vshCommandOptString(cmd,
> "snapshot-source-vol", &found);
> +    snapshotStrFormat = vshCommandOptString(cmd,
> "snapshot-source-format", &found);
>       virBufferAddLit(&buf, "<volume>\n");
>      virBufferVSprintf(&buf, "  <name>%s</name>\n", name);
> @@ -5351,8 +5356,53 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
>          virBufferVSprintf(&buf, "    <format type='%s'/>\n",format);
>          virBufferAddLit(&buf, "  </target>\n");
>      }
> -    virBufferAddLit(&buf, "</volume>\n");
>  +    // Convert the snapshot parameters into backingStore XML
> +    if (snapshotStrVol) {
> +        // Use the name of the snapshot source volume to retrieve
> its device path
> +        vshDebug(ctl, 5, "%s: Looking up backing store volume '%s'
> as name\n", cmd->def->name, snapshotStrVol);
> +        virStorageVolPtr snapVol = virStorageVolLookupByName(pool,
> snapshotStrVol);

We don't typically mix declarations and code.  It's actually not entirely
true (you'll see examples of it if you look around hard enough), but it's
kind of surprising to see new variables in the middle of blocks of code.
Please move this to the top.

> +        if (snapVol)
> +                vshDebug(ctl, 5, "%s: Backing store volume found
> using '%s' as name\n", cmd->def->name, snapshotStrVol);
> +
> +        if (snapVol == NULL) {
> +            // Snapshot source volume was not found by name.  Try
> using the snapshot-source-vol parameter as a key
> +            vshDebug(ctl, 5, "%s: Looking up backing store volume
> '%s' as key\n", cmd->def->name, snapshotStrVol);
> +            snapVol = virStorageVolLookupByKey(ctl->conn, snapshotStrVol);
> +            if (snapVol)
> +                vshDebug(ctl, 5, "%s: Backing store volume found
> using '%s' as key\n", cmd->def->name, snapshotStrVol);
> +        }
> +        if (snapVol == NULL) {
> +            // Snapshot source volume was not found by key either.
> Try using the snapshot-source-vol parameter as a path
> +            vshDebug(ctl, 5, "%s: Looking up backing store volume
> '%s' by path\n", cmd->def->name, snapshotStrVol);
> +            snapVol = virStorageVolLookupByPath(ctl->conn, snapshotStrVol);
> +            if (snapVol)
> +                vshDebug(ctl, 5, "%s: Backing store volume found
> using '%s' as path\n", cmd->def->name, snapshotStrVol);
> +        }
> +        if (snapVol == NULL) {
> +            vshError(ctl, _("failed to get vol '%s'"), snapshotStrVol);
> +            return FALSE;
> +        }
> +
> +        char *snapshotStrVolPath;
> +        if ((snapshotStrVolPath = virStorageVolGetPath(snapVol)) == NULL) {
> +            virStorageVolFree(snapVol);
> +            return FALSE;
> +        }
> +
> +        // Create the XML for the backing store
> +        virBufferAddLit(&buf, "  <backingStore>\n");
> +        virBufferVSprintf(&buf, "
> <path>%s</path>\n",snapshotStrVolPath);

I'm not entirely certain because of the broken patch, but it seems like you
need to add 4 spaces before <path>.

> +        if (snapshotStrFormat)
> +            virBufferVSprintf(&buf, "    <format
> type='%s'/>\n",snapshotStrFormat);

Here, you'll have to handle the case where the user passed in NULL for
snapshotStrFormat.

> +        virBufferAddLit(&buf, "  </backingStore>\n");
> +
> +        // Cleanup snapshot allocations
> +        VIR_FREE(snapshotStrVolPath);
> +        virStorageVolFree(snapVol);
> +    }
> +
> +    virBufferAddLit(&buf, "</volume>\n");

Overall, it looks OK.  I'm not a huge fan of the separation of
--snapshot-source-vol and --snapshot-source-format, since it makes it too
easy to mess up one or the other.  On the other hand, it is a bit easier than
making XML by hand, and I don't have any better ideas, so maybe we should
leave it as-is.  If we do leave it, I think you'll want to add some code to
check for all 4 cases of:

snapshotStrVol == NULL, snapshotStrFormat == NULL
snapshotStrVol == specified, snapshotStrFormat == NULL
snapshotStrVol == specified, snapshotStrFormat == specified
snapshotStrVol == NULL, snapshotStrFormat == specified

--
Chris Lalancette