7:34 a.m.
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
10:33 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:09 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:46 a.m.
New subject: [libvirt] [PATCH] virsh: allow empty string arguments
"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
3:13 p.m.
New subject: [libvirt] [PATCH] virsh: allow empty string arguments
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
3:40 p.m.
New subject: [libvirt] [PATCH] virsh: allow empty string arguments
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5002
days inactive
5003
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Michal Privoznik