[libvirt] [PATCH] virsh: Insert error messages to avoid a quiet abortion of commands
in case of incorrect option parsing. --- My former patch introduced a regression: https://bugzilla.redhat.com/show_bug.cgi?id=605660 tools/virsh.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 38 insertions(+), 14 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b42aac4..42ebd55 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -706,8 +706,10 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) } VIR_FREE(ctl->name); - if (vshCommandOptString(cmd, "name", &name) <= 0) + if (vshCommandOptString(cmd, "name", &name) < 0) { + vshError(ctl, "%s", _("Please specify valid connection URI")); return FALSE; + } ctl->name = vshStrdup(ctl, name); if (!ro) { @@ -773,7 +775,7 @@ static int cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int ret; + int ret = FALSE; const char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -782,11 +784,14 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - if (vshCommandOptString(cmd, "devname", &name) < 0) - return FALSE; + if (vshCommandOptString(cmd, "devname", &name) < 0) { + vshError(ctl, "%s", _("Invalid devname")); + goto cleanup; + } ret = cmdRunConsole(ctl, dom, name); +cleanup: virDomainFree(dom); return ret; } @@ -2410,8 +2415,10 @@ cmdMaxvcpus(vshControl *ctl, const vshCmd *cmd) const char *type = NULL; int vcpus; - if (vshCommandOptString(cmd, "type", &type) < 0) + if (vshCommandOptString(cmd, "type", &type) < 0) { + vshError(ctl, "%s", _("Invalid type")); return FALSE; + } if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -2856,8 +2863,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - if (vshCommandOptInt(cmd, "count", &count) < 0) - return FALSE; + if (vshCommandOptInt(cmd, "count", &count) < 0) { + vshError(ctl, "%s", _("Invalid number of virtual CPUs")); + goto cleanup; + } if (!flags) { if (virDomainSetVcpus(dom, count) != 0) { @@ -3705,8 +3714,10 @@ doMigrate (void *opaque) if (vshCommandOptString(cmd, "desturi", &desturi) <= 0 || vshCommandOptString(cmd, "migrateuri", &migrateuri) < 0 || - vshCommandOptString(cmd, "dname", &dname) < 0) + vshCommandOptString(cmd, "dname", &dname) < 0) { + vshError(ctl, "%s", _("missing argument")); goto out; + } if (vshCommandOptBool (cmd, "live")) flags |= VIR_MIGRATE_LIVE; @@ -5509,8 +5520,10 @@ static int buildPoolXML(const vshCmd *cmd, const char **retname, char **xml) { vshCommandOptString(cmd, "source-dev", &srcDev) < 0 || vshCommandOptString(cmd, "source-name", &srcName) < 0 || vshCommandOptString(cmd, "source-format", &srcFormat) < 0 || - vshCommandOptString(cmd, "target", &target) < 0) + vshCommandOptString(cmd, "target", &target) < 0) { + vshError(NULL, "%s", _("missing argument")); goto cleanup; + } virBufferVSprintf(&buf, "<pool type='%s'>\n", type); virBufferVSprintf(&buf, " <name>%s</name>\n", name); @@ -6314,8 +6327,10 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (vshCommandOptString(cmd, "type", &type) <= 0 || vshCommandOptString(cmd, "host", &host) < 0 || - vshCommandOptString(cmd, "initiator", &initiator) < 0) + vshCommandOptString(cmd, "initiator", &initiator) < 0) { + vshError(ctl,"%s", _("missing argument")); return FALSE; + } if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -6386,8 +6401,10 @@ cmdPoolDiscoverSources(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (vshCommandOptString(cmd, "type", &type) <= 0) return FALSE; - if (vshCommandOptString(cmd, "srcSpec", &srcSpecFile) < 0) + if (vshCommandOptString(cmd, "srcSpec", &srcSpecFile) < 0) { + vshError(ctl, "%s", _("missing option")); return FALSE; + } if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -8862,8 +8879,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) vshCommandOptString(cmd, "target", &target) < 0 || vshCommandOptString(cmd, "mac", &mac) < 0 || vshCommandOptString(cmd, "script", &script) < 0 || - vshCommandOptString(cmd, "model", &model) < 0) + vshCommandOptString(cmd, "model", &model) < 0) { + vshError(ctl, "missing argument"); goto cleanup; + } /* check interface type */ if (STREQ(type, "network")) { @@ -8969,8 +8988,10 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "type", &type) <= 0) goto cleanup; - if (vshCommandOptString(cmd, "mac", &mac) < 0) + if (vshCommandOptString(cmd, "mac", &mac) < 0) { + vshError(ctl, "%s", _("missing option")); goto cleanup; + } doc = virDomainGetXMLDesc(dom, 0); if (!doc) @@ -9120,6 +9141,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) vshCommandOptString(cmd, "type", &type) < 0 || vshCommandOptString(cmd, "mode", &mode) < 0 || vshCommandOptString(cmd, "sourcetype", &stype) < 0) { + vshError(ctl, "%s", _("missing option")); goto cleanup; } @@ -11272,8 +11294,10 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - if (vshCommandOptString(cmd, pooloptname, &p) < 0) + if (vshCommandOptString(cmd, pooloptname, &p) < 0) { + vshError(ctl, "%s", _("missing option")); return NULL; + } if (p) pool = vshCommandOptPoolBy(ctl, cmd, pooloptname, name, flag); -- 1.7.4
On 03/14/2011 06:34 AM, Michal Privoznik wrote:
in case of incorrect option parsing. --- My former patch introduced a regression: https://bugzilla.redhat.com/show_bug.cgi?id=605660
tools/virsh.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index b42aac4..42ebd55 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -706,8 +706,10 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) }
VIR_FREE(ctl->name); - if (vshCommandOptString(cmd, "name", &name) <= 0) + if (vshCommandOptString(cmd, "name", &name) < 0) { + vshError(ctl, "%s", _("Please specify valid connection URI")); return FALSE; + }
ACK, and sorry for not catching that on the first review. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/14/2011 09:33 AM, Eric Blake wrote:
On 03/14/2011 06:34 AM, Michal Privoznik wrote:
in case of incorrect option parsing. --- My former patch introduced a regression: https://bugzilla.redhat.com/show_bug.cgi?id=605660
tools/virsh.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index b42aac4..42ebd55 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -706,8 +706,10 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) }
VIR_FREE(ctl->name); - if (vshCommandOptString(cmd, "name", &name) <= 0) + if (vshCommandOptString(cmd, "name", &name) < 0) {
Actually, all your other changes were where pre-patch had < 0, so you were already prepared to deal with an optional argument. But for cmdConnect, you changed <= 0 to < 0, you now go on to the rest of the method with name still NULL, which causes problems on the subsequent strdup(). But an empty string for the URI is valid (it means the same as a NULL URI for requesting the default connection). Yet vshCommandOptString rejects the empty string. I'm pushing your patch as-is, but we need another followup for commands that specifically want to handle the empty string (such as cmdConnect) in a manner other than just rejecting it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
"virsh connect ''" should try to connect to the default connection, but the previous patch made it issue a warning about an invalid URI. * tools/virsh.c (VSH_OFLAG_EMPTY_OK): New option flag. (vshCommandOptString): Per the declaration, value is required to be non-NULL. Honor new flag. (opts_connect): Allow empty string connection. --- tools/virsh.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 42ebd55..ef0cfea 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -152,8 +152,9 @@ typedef enum { /* * Command Option Flags */ -#define VSH_OFLAG_NONE 0 /* without flags */ -#define VSH_OFLAG_REQ (1 << 1) /* option required */ +#define VSH_OFLAG_NONE 0 /* without flags */ +#define VSH_OFLAG_REQ (1 << 1) /* option required */ +#define VSH_OFLAG_EMPTY_OK (1 << 2) /* empty string option allowed */ /* dummy */ typedef struct __vshControl vshControl; @@ -685,7 +686,8 @@ static const vshCmdInfo info_connect[] = { }; static const vshCmdOptDef opts_connect[] = { - {"name", VSH_OT_DATA, 0, N_("hypervisor connection URI")}, + {"name", VSH_OT_DATA, VSH_OFLAG_EMPTY_OK, + N_("hypervisor connection URI")}, {"readonly", VSH_OT_BOOL, 0, N_("read-only connection")}, {NULL, 0, 0, NULL} }; @@ -10993,14 +10995,16 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) int ret = 0; if (arg && arg->data) { - ret = -1; - if (*arg->data) { - if (value) { - *value = arg->data; - ret = 1; - } + if (*arg->data + || (arg->def && (arg->def->flag & VSH_OFLAG_EMPTY_OK))) { + *value = arg->data; + ret = 1; } else if (arg->def && ((arg->def->flag) & VSH_OFLAG_REQ)) { vshError(NULL, _("Missing required option '%s'"), name); + ret = -1; + } else { + /* Treat "--option ''" as if option had not been specified. */ + ret = 0; } } -- 1.7.4
On Mon, Mar 14, 2011 at 10:46:07 -0600, Eric Blake wrote:
"virsh connect ''" should try to connect to the default connection, but the previous patch made it issue a warning about an invalid URI.
* tools/virsh.c (VSH_OFLAG_EMPTY_OK): New option flag. (vshCommandOptString): Per the declaration, value is required to be non-NULL. Honor new flag. (opts_connect): Allow empty string connection. --- tools/virsh.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-)
ACK Jirka
On 03/15/2011 02:13 PM, Jiri Denemark wrote:
On Mon, Mar 14, 2011 at 10:46:07 -0600, Eric Blake wrote:
"virsh connect ''" should try to connect to the default connection, but the previous patch made it issue a warning about an invalid URI.
* tools/virsh.c (VSH_OFLAG_EMPTY_OK): New option flag. (vshCommandOptString): Per the declaration, value is required to be non-NULL. Honor new flag. (opts_connect): Allow empty string connection. --- tools/virsh.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-)
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Michal Privoznik