[libvirt] [PATCH] virsh: Report error when taking a snapshot with empty --memspec argument

[Martin Kletzander]

On 11/22/2012 11:03 AM, Peter Krempa wrote:
> When the value of memspec was empty taking of a snapshot failed without
> reporting an error.
> ---
> This is only a quick fix. I think we should improve vshCommandOptStr to detect
> this for us and report an appropriate error, but this change will require
> a lot of changes not relevant to this problem.

I don't think we need to improve that.  The function already behaves
depending on the VSH_OFLAG_* being set and returns the right value based
on that.  If you meant setting an error, that could be achieved, but
it's already handled in down the stack and even though rewriting it
could save up to 5 lines of code ( :) ), it's better to leave it this
way IMHO.

> ---
>  tools/virsh-snapshot.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 398730c..057ae2d 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -358,7 +358,12 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
>      if (desc)
>          virBufferEscapeString(&buf, "  <description>%s</description>\n", desc);
> 
> -    if (vshCommandOptString(cmd, "memspec", &memspec) < 0 ||
> +    if (vshCommandOptString(cmd, "memspec", &memspec) < 0) {
> +        vshError(ctl, _("memspec argument must not be empty"));
> +        goto cleanup;
> +    }
> +
> +    if (memspec &&
>          vshParseSnapshotMemspec(ctl, &buf, memspec) < 0) {
>          virBufferFreeAndReset(&buf);
>          goto cleanup;
> 

However, ACK to this change,

Martin