5:10 a.m.
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
5:10 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: Reorder some options
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(a)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
5:10 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: Enforce proper ordering of options
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(a)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
5:10 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Error out if VSH_OT_STRING option has VSH_OFLAG_REQ flag
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(a)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
8:08 a.m.
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
3662
days inactive
3664
days old
4 comments
2 participants
participants (2)
-
Martin Kletzander -
Michal Privoznik