[libvirt] [PATCH 0/3] virsh: Cleanups and checks for options
Option-ordering and typing checks and a reorder so tests pass. Martin Kletzander (3): virsh: Reorder some options virsh: Enforce proper ordering of options virsh: Error out if VSH_OT_STRING option has VSH_OFLAG_REQ flag tools/virsh-domain.c | 76 ++++++++++++++++++++++++++-------------------------- tools/virsh-pool.c | 8 +++--- tools/virsh-volume.c | 8 +++--- tools/virsh.c | 11 +++++++- 4 files changed, 56 insertions(+), 47 deletions(-) -- 2.1.3
According to comments in parsing functions, optional options should be specified *after* required ones. It makes sense and help output looks cleaner. The only exceptions are options with type == VSH_OT_ARGV. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 76 ++++++++++++++++++++++++++-------------------------- tools/virsh-pool.c | 8 +++--- tools/virsh-volume.c | 8 +++--- 3 files changed, 46 insertions(+), 46 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8e743f1..541363d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3222,11 +3222,6 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { .flags = VSH_OFLAG_REQ, .help = N_("domain name, id or uuid") }, - {.name = "duration", - .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, - .help = N_("duration in seconds") - }, {.name = "target", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3234,6 +3229,11 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { "disk(Suspend-to-Disk), " "hybrid(Hybrid-Suspend)") }, + {.name = "duration", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("duration in seconds") + }, {.name = NULL} }; @@ -3940,10 +3940,6 @@ static const vshCmdInfo info_save[] = { }; static const vshCmdOptDef opts_save[] = { - {.name = "bypass-cache", - .type = VSH_OT_BOOL, - .help = N_("avoid file system cache when saving") - }, {.name = "domain", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3954,6 +3950,10 @@ static const vshCmdOptDef opts_save[] = { .flags = VSH_OFLAG_REQ, .help = N_("where to save the data") }, + {.name = "bypass-cache", + .type = VSH_OT_BOOL, + .help = N_("avoid file system cache when saving") + }, {.name = "xml", .type = VSH_OT_STRING, .help = N_("filename containing updated XML for the target") @@ -4408,15 +4408,15 @@ static const vshCmdInfo info_managedsave[] = { }; static const vshCmdOptDef opts_managedsave[] = { - {.name = "bypass-cache", - .type = VSH_OT_BOOL, - .help = N_("avoid file system cache when saving") - }, {.name = "domain", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, .help = N_("domain name, id or uuid") }, + {.name = "bypass-cache", + .type = VSH_OT_BOOL, + .help = N_("avoid file system cache when saving") + }, {.name = "running", .type = VSH_OT_BOOL, .help = N_("set domain to be running on next start") @@ -4918,6 +4918,16 @@ static const vshCmdInfo info_dump[] = { }; static const vshCmdOptDef opts_dump[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "file", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("where to dump the core") + }, {.name = "live", .type = VSH_OT_BOOL, .help = N_("perform a live core dump if supported") @@ -4934,16 +4944,6 @@ static const vshCmdOptDef opts_dump[] = { .type = VSH_OT_BOOL, .help = N_("reset the domain after core dump") }, - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("where to dump the core") - }, {.name = "verbose", .type = VSH_OT_BOOL, .help = N_("display the progress of dump") @@ -7452,6 +7452,11 @@ static const vshCmdOptDef opts_metadata[] = { .flags = VSH_OFLAG_REQ, .help = N_("domain name, id or uuid") }, + {.name = "uri", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("URI of the namespace") + }, {.name = "live", .type = VSH_OT_BOOL, .help = N_("modify/get running state") @@ -7468,11 +7473,6 @@ static const vshCmdOptDef opts_metadata[] = { .type = VSH_OT_BOOL, .help = N_("use an editor to change the metadata") }, - {.name = "uri", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("URI of the namespace") - }, {.name = "key", .type = VSH_OT_DATA, .help = N_("key to be used as a namespace identifier"), @@ -9269,6 +9269,16 @@ static const vshCmdInfo info_migrate[] = { }; static const vshCmdOptDef opts_migrate[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "desturi", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("connection URI of the destination host as seen from the client(normal migration) or source(p2p migration)") + }, {.name = "live", .type = VSH_OT_BOOL, .help = N_("live migration") @@ -9341,16 +9351,6 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_BOOL, .help = N_("abort on soft errors during migration") }, - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, - {.name = "desturi", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("connection URI of the destination host as seen from the client(normal migration) or source(p2p migration)") - }, {.name = "migrateuri", .type = VSH_OT_DATA, .help = N_("migration URI, usually can be omitted") diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 0b8dba5..31f7ec3 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -190,15 +190,15 @@ static const vshCmdOptDef opts_pool_X_as[] = { .flags = VSH_OFLAG_REQ, .help = N_("name of the pool") }, - {.name = "print-xml", - .type = VSH_OT_BOOL, - .help = N_("print XML document, but don't define/create") - }, {.name = "type", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, .help = N_("type of the pool") }, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document, but don't define/create") + }, {.name = "source-host", .type = VSH_OT_DATA, .help = N_("source-host for underlying storage") diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 4f810f8..66d71aa 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1621,15 +1621,15 @@ static const vshCmdInfo info_vol_pool[] = { }; static const vshCmdOptDef opts_vol_pool[] = { - {.name = "uuid", - .type = VSH_OT_BOOL, - .help = N_("return the pool uuid rather than pool name") - }, {.name = "vol", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, .help = N_("volume key or path") }, + {.name = "uuid", + .type = VSH_OT_BOOL, + .help = N_("return the pool uuid rather than pool name") + }, {.name = NULL} }; -- 2.1.3
Even though vshCmddefOptParse() tried returning -1 if there was an optional option specification that preceded a required one, it failed to check that for boolean type options and options with VSH_OFLAG_REQ_OPT flag set. On the other hand, it makes sense that VSH_OT_ARGV is specified at the end of the option list. Returning -1 enforces the proper ordering thanks to virsh-synopsis test in 'make check'. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/virsh.c b/tools/virsh.c index 036b517..41893bb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1073,6 +1073,7 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, if (i > 31) return -1; /* too many options */ if (opt->type == VSH_OT_BOOL) { + optional = true; if (opt->flags & VSH_OFLAG_REQ) return -1; /* bool options can't be mandatory */ continue; @@ -1105,12 +1106,14 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, if (opt->flags & VSH_OFLAG_REQ_OPT) { if (opt->flags & VSH_OFLAG_REQ) *opts_required |= 1 << i; + else + optional = true; continue; } *opts_need_arg |= 1 << i; if (opt->flags & VSH_OFLAG_REQ) { - if (optional) + if (optional && opt->type != VSH_OT_ARGV) return -1; /* mandatory options must be listed first */ *opts_required |= 1 << i; } else { -- 2.1.3
Recent commit 12bd207e217f3c5dc2272a5ea943b81067bd8034 fixed few VSH_OT_STRING options that should've been VSH_OT_DATA. That lead me to this commit that enforces people to check that newly added options have proper type. Thanks to virsh erroring out with error message, this will immediately show up in 'make check' thanks to our virsh-synopsis test. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index 41893bb..e10b3de 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1386,6 +1386,12 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) break; case VSH_OT_STRING: /* OT_STRING should never be VSH_OFLAG_REQ */ + if (opt->flags & VSH_OFLAG_REQ) { + vshError(ctl, + _("internal error: bad options in command: '%s'"), + def->name); + return false; + } snprintf(buf, sizeof(buf), _("--%s <string>"), opt->name); break; case VSH_OT_DATA: -- 2.1.3
On 11.11.2014 12:10, Martin Kletzander wrote:
Option-ordering and typing checks and a reorder so tests pass.
Martin Kletzander (3): virsh: Reorder some options virsh: Enforce proper ordering of options virsh: Error out if VSH_OT_STRING option has VSH_OFLAG_REQ flag
tools/virsh-domain.c | 76 ++++++++++++++++++++++++++-------------------------- tools/virsh-pool.c | 8 +++--- tools/virsh-volume.c | 8 +++--- tools/virsh.c | 11 +++++++- 4 files changed, 56 insertions(+), 47 deletions(-)
ACK series. Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik