[virt-tools-list] virt-install: Add warn if 'single-use' options are defined multiple times?
Mark Kanda
mark.kanda at oracle.com
Thu Oct 4 16:29:32 UTC 2018
On 10/1/2018 12:16 PM, Cole Robinson wrote:
> On 09/14/2018 04:28 PM, Mark Kanda wrote:
>> Hi all,
>>
>> I recently discovered, for 'single-use' command line options,
>> virt-install silently ignores all but the last definition. For
>> example, if the command line has '--memory X' and later '--memory Y',
>> then 'X' is silently ignored.
>>
>> I think virt-install should warn the user if 'single use' options are
>> specified multiple times, and I'd like to implement a fix.
>>
>> However, before I implement such a fix, I'd like to know if this is
>> 'by design', or if anyone would otherwise object to such a check..
>>
>
> Sorry for the late response. This isn't really intentional and more just
> a side effect of how our argparse options are initialized. I hacked up
> the attached diff that will essentially merge all ex. --memory options,
> so --memory 123,maxmem=456 --memory 555 will be equivalent to --memory
> 555,maxmem=456. Is that fine for you needs?
>
> I think it's a bit nicer and has benefits if you want to pass in an
> extra option to a virt-install script or similar
>
Thanks Cole,
Yes, this does look better. However, IMO, the user should be warned if
they do something like '--memory 123,maxmem=456 --memory 555' because
it's not clear if they want '123' or '555'.
I was thinking of defining and using a custom argpase action to warn the
user - see attached patch (incomplete - likely missing many single-use
args).
What do you think of this approach?
Thanks,
-Mark
-------------- next part --------------
>From 39da4228933e49f49a76078e38c245b7a4e5cb7f Mon Sep 17 00:00:00 2001
From: Mark Kanda <mark.kanda at oracle.com>
Date: Thu, 4 Oct 2018 10:36:24 -0500
Subject: [PATCH virt-manager 1/1] Add warning if single-use options are used
multiple times
Add warning if single-use options are used multiple times to
prevent unintentional user errors.
Signed-off-by: Mark Kanda <mark.kanda at oracle.com>
---
virt-install | 23 +++++++++++++++--------
virtinst/cli.py | 36 ++++++++++++++++++++++++++----------
2 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/virt-install b/virt-install
index 4163d0f..ee1c9d8 100755
--- a/virt-install
+++ b/virt-install
@@ -28,7 +28,7 @@ import libvirt
import virtinst
from virtinst import cli
-from virtinst.cli import fail, print_stdout, print_stderr
+from virtinst.cli import fail, print_stdout, print_stderr, SingleOptionCheck
##############################
@@ -852,7 +852,8 @@ def parse_args():
geng = parser.add_argument_group(_("General Options"))
geng.add_argument("-n", "--name",
- help=_("Name of the guest instance"))
+ help=_("Name of the guest instance"),
+ action=SingleOptionCheck)
cli.add_memory_option(geng, backcompat=True)
cli.vcpu_cli_options(geng)
cli.add_metadata_option(geng)
@@ -880,10 +881,12 @@ def parse_args():
# Takes a URL and just prints to stdout the detected distro name
insg.add_argument("--test-media-detection", help=argparse.SUPPRESS)
- insg.add_argument("--os-type", dest="distro_type", help=argparse.SUPPRESS)
+ insg.add_argument("--os-type", dest="distro_type", help=argparse.SUPPRESS,
+ action=SingleOptionCheck)
insg.add_argument("--os-variant", dest="distro_variant",
help=_("The OS variant being installed guests, "
- "e.g. 'fedora18', 'rhel6', 'winxp', etc."))
+ "e.g. 'fedora18', 'rhel6', 'winxp', etc."),
+ action=SingleOptionCheck)
cli.add_boot_options(insg)
insg.add_argument("--init", help=argparse.SUPPRESS)
@@ -932,13 +935,16 @@ def parse_args():
help=_("This guest should be a container guest"))
virg.add_argument("--virt-type", dest="hv_type",
default="",
- help=_("Hypervisor name to use (kvm, qemu, xen, ...)"))
+ help=_("Hypervisor name to use (kvm, qemu, xen, ...)"),
+ action=SingleOptionCheck)
virg.add_argument("--accelerate", action="store_true", default=False,
help=argparse.SUPPRESS)
virg.add_argument("--arch",
- help=_("The CPU architecture to simulate"))
+ help=_("The CPU architecture to simulate"),
+ action=SingleOptionCheck)
virg.add_argument("--machine",
- help=_("The machine type to emulate"))
+ help=_("The machine type to emulate"),
+ action=SingleOptionCheck)
virg.add_argument("--noapic", action="store_true",
default=False, help=argparse.SUPPRESS)
virg.add_argument("--noacpi", action="store_true",
@@ -953,7 +959,8 @@ def parse_args():
default=False,
help=_("Create a transient domain."))
misc.add_argument("--wait", type=int, dest="wait",
- help=_("Minutes to wait for install to complete."))
+ help=_("Minutes to wait for install to complete."),
+ action=SingleOptionCheck)
cli.add_misc_options(misc, prompt=True, printxml=True, printstep=True,
noreboot=True, dryrun=True, noautoconsole=True)
diff --git a/virtinst/cli.py b/virtinst/cli.py
index 3d3ac0a..28e61a1 100644
--- a/virtinst/cli.py
+++ b/virtinst/cli.py
@@ -521,10 +521,12 @@ def get_meter():
def add_connect_option(parser, invoker=None):
if invoker == "virt-xml":
parser.add_argument("-c", "--connect", metavar="URI",
- help=_("Connect to hypervisor with libvirt URI"))
+ help=_("Connect to hypervisor with libvirt URI"),
+ action=SingleOptionCheck)
else:
parser.add_argument("--connect", metavar="URI",
- help=_("Connect to hypervisor with libvirt URI"))
+ help=_("Connect to hypervisor with libvirt URI"),
+ action=SingleOptionCheck)
def add_misc_options(grp, prompt=False, replace=False,
@@ -592,7 +594,8 @@ def add_metadata_option(grp):
grp.add_argument("--metadata",
help=_("Configure guest metadata. Ex:\n"
"--metadata name=foo,title=\"My pretty title\",uuid=...\n"
- "--metadata description=\"My nice long description\""))
+ "--metadata description=\"My nice long description\""),
+ action=SingleOptionCheck)
def add_memory_option(grp, backcompat=False):
@@ -601,10 +604,12 @@ def add_memory_option(grp, backcompat=False):
"--memory 1024 (in MiB)\n"
"--memory 512,maxmemory=1024\n"
"--memory 512,maxmemory=1024,hotplugmemorymax=2048,"
- "hotplugmemoryslots=2"))
+ "hotplugmemoryslots=2"),
+ action=SingleOptionCheck)
if backcompat:
grp.add_argument("-r", "--ram", type=int, dest="oldmemory",
- help=argparse.SUPPRESS)
+ help=argparse.SUPPRESS,
+ action=SingleOptionCheck)
def vcpu_cli_options(grp, backcompat=True, editexample=False):
@@ -612,7 +617,8 @@ def vcpu_cli_options(grp, backcompat=True, editexample=False):
help=_("Number of vcpus to configure for your guest. Ex:\n"
"--vcpus 5\n"
"--vcpus 5,maxcpus=10,cpuset=1-4,6,8\n"
- "--vcpus sockets=2,cores=4,threads=2,"))
+ "--vcpus sockets=2,cores=4,threads=2,"),
+ action=SingleOptionCheck)
extramsg = "--cpu host"
if editexample:
@@ -620,7 +626,8 @@ def vcpu_cli_options(grp, backcompat=True, editexample=False):
grp.add_argument("--cpu",
help=_("CPU model and features. Ex:\n"
"--cpu coreduo,+x2apic\n"
- "--cpu host-passthrough\n") + extramsg)
+ "--cpu host-passthrough\n") + extramsg,
+ action=SingleOptionCheck)
if backcompat:
grp.add_argument("--check-cpu", action="store_true",
@@ -735,9 +742,11 @@ def add_guest_xml_options(geng):
"--features apic=on,eoi=on"))
geng.add_argument("--clock",
help=_("Set domain <clock> XML. Ex:\n"
- "--clock offset=localtime,rtc_tickpolicy=catchup"))
+ "--clock offset=localtime,rtc_tickpolicy=catchup"),
+ action=SingleOptionCheck)
geng.add_argument("--pm",
- help=_("Configure VM power management features"))
+ help=_("Configure VM power management features"),
+ action=SingleOptionCheck)
geng.add_argument("--events",
help=_("Configure VM lifecycle management policy"))
geng.add_argument("--resource", action="append",
@@ -759,7 +768,8 @@ def add_boot_options(insg):
insg.add_argument("--boot",
help=_("Configure guest boot settings. Ex:\n"
"--boot hd,cdrom,menu=on\n"
- "--boot init=/sbin/init (for containers)"))
+ "--boot init=/sbin/init (for containers)"),
+ action=SingleOptionCheck)
insg.add_argument("--idmap",
help=_("Enable user namespace for LXC container. Ex:\n"
"--idmap uid_start=0,uid_target=1000,uid_count=10"))
@@ -1283,6 +1293,12 @@ class VirtCLIParser(object):
return ret
+class SingleOptionCheck(argparse.Action):
+ def __call__(self, parser, namespace, values, option_string=None):
+ if getattr(namespace, self.dest, None) is not None:
+ logging.warning("Warning: option '%s' used multiple times" %
+ option_string)
+ setattr(namespace, self.dest, values)
VIRT_PARSERS = []
--
1.8.3.1
More information about the virt-tools-list
mailing list