[virt-tools-list] [PATCH] Error out cleanly when autoclone or destination file is not specified

[Cole Robinson]

On 08/19/2016 03:53 AM, Kothapally Madhu Pavan wrote:
> When virt-clone is used without autoclone or destination file, libvirt
> errors with message "ERROR    missing source information for device vda".
> This doesn't convey what is missing. This patch will indicate which
> options to use.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  tests/clitest.py |    4 ++--
>  virt-clone       |    7 ++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/clitest.py b/tests/clitest.py
> index 4bcd85c..8009082 100644
> --- a/tests/clitest.py
> +++ b/tests/clitest.py
> @@ -930,7 +930,7 @@ c.add_invalid("-o test-clone-simple -n newvm --file %(EXISTIMG1)s --clone-runnin
>  
>  
>  c = vclon.add_category("general", "-n clonetest")
> -c.add_valid("-o test")  # Nodisk guest
> +c.add_valid("-o test --auto-clone")  # Auto flag, no storage
>  c.add_valid("-o test --file %(NEWCLONEIMG1)s --file %(NEWCLONEIMG2)s")  # Nodisk, but with spurious files passed
>  c.add_valid("-o test --file %(NEWCLONEIMG1)s --file %(NEWCLONEIMG2)s --prompt")  # Working scenario w/ prompt shouldn't ask anything
>  c.add_valid("--original-xml %(CLONE_DISK_XML)s --file %(NEWCLONEIMG1)s --file %(NEWCLONEIMG2)s")  # XML File with 2 disks
> @@ -939,7 +939,7 @@ c.add_valid("--original-xml %(CLONE_DISK_XML)s --file %(NEWCLONEIMG1)s --file %(
>  c.add_valid("--original-xml %(CLONE_DISK_XML)s --file %(NEWCLONEIMG1)s --file %(NEWCLONEIMG2)s --force-copy=fda")  # XML w/ disks, force copy a target with no media
>  c.add_valid("--original-xml %(CLONE_STORAGE_XML)s --file %(MANAGEDNEW1)s")  # XML w/ managed storage, specify managed path
>  c.add_valid("--original-xml %(CLONE_NOEXIST_XML)s --file %(EXISTIMG1)s --preserve")  # XML w/ managed storage, specify managed path across pools# Libvirt test driver doesn't support cloning across pools# XML w/ non-existent storage, with --preserve
> -c.add_valid("-o test -n test-for-clone --replace")  # Overwriting existing VM
> +c.add_valid("-o test -n test-for-clone --auto-clone --replace")  # Overwriting existing VM
>  c.add_invalid("-o test foobar")  # Positional arguments error
>  c.add_invalid("-o idontexist")  # Non-existent vm name
>  c.add_invalid("-o idontexist --auto-clone")  # Non-existent vm name with auto flag,
> diff --git a/virt-clone b/virt-clone
> index 00d0e30..b689d9a 100755
> --- a/virt-clone
> +++ b/virt-clone
> @@ -112,7 +112,7 @@ def parse_args():
>                             "The status must be shut off or paused."))
>      geng.add_argument("--original-xml",
>                      help=_("XML file to use as the original guest."))
> -    geng.add_argument("--auto-clone", action="store_true",
> +    geng.add_argument("--auto-clone", dest="auto_clone", action="store_true",
>                      help=_("Auto generate clone name and storage paths from"
>                             " the original guest configuration."))

This bit isn't required, that's the default parameter name, so I dropped this
chunk

>      geng.add_argument("-n", "--name", dest="new_name",
> @@ -168,6 +168,11 @@ def main(conn=None):
>      if conn is None:
>          conn = cli.getConnection(options.connect)
>  
> +    if (options.new_diskfile is None and options.auto_clone is False and
> +        options.xmlonly is False):
> +        fail(_("Either --auto-clone or --file is required,"
> +             " use '--auto-clone or --file' and try again."))
> +

Tweaked the spacing here and pushed, thanks! Though I've added a todo item for
myself to make --auto-clone the implied default, but it will take a bit of work.

Thanks and sorry for the delay.

Cole