1:20 p.m.
On 03/07/2011 11:46 AM, Michal Privoznik wrote:
This is needed to detect situations when optional argument was
specified with non-integer value: '--int-opt foo'. To keep functions
uniform vshCommandOptString function was also changed, because it
returns tri-state value as well. Given result pointer is updated only
in case of success. If parsing fails, result is not updated at all.
diff --git a/tools/virsh.c b/tools/virsh.c
index f3754d7..c274a6b 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -252,13 +252,14 @@ static const vshCmdGrp *vshCmdGrpSearch(const char *grpname);
static int vshCmdGrpHelp(vshControl *ctl, const char *name);
static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name);
-static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found);
-static unsigned long vshCommandOptUL(const vshCmd *cmd, const char *name,
- int *found);
-static char *vshCommandOptString(const vshCmd *cmd, const char *name,
- int *found);
Ouch - this was not const-correct pre-patch, but at least assigning
'char *' to 'const char *' is trivial...
-static long long vshCommandOptLongLong(const vshCmd *cmd, const char
*name,
- int *found);
+static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
+ ATTRIBUTE_NONNULL(3);
+static int vshCommandOptUL(const vshCmd *cmd, const char *name,
+ unsigned long *value) ATTRIBUTE_NONNULL(3);
+static int vshCommandOptString(const vshCmd *cmd, const char *name,
+ char **value) ATTRIBUTE_NONNULL(3);
whereas this rewrite exposes the const-correctness issue - you can't
pass 'const char* var; &var' to a 'char **' argument and still keep
the
compiler happy...
+static int vshCommandOptLongLong(const vshCmd *cmd, const char
*name,
+ unsigned long long *value) ATTRIBUTE_NONNULL(3);
static int vshCommandOptBool(const vshCmd *cmd, const char *name);
static char *vshCommandOptArgv(const vshCmd *cmd, int count);
@@ -589,9 +590,9 @@ static const vshCmdOptDef opts_help[] = {
static int
cmdHelp(vshControl *ctl, const vshCmd *cmd)
{
- const char *name;
+ char *name = NULL;
- name = vshCommandOptString(cmd, "command", NULL);
+ vshCommandOptString(cmd, "command", &name);
...leading to instances like this where you nukes const designators...
@@ -773,7 +773,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
{
virDomainPtr dom;
int ret;
- const char *name;
+ const char *name = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
return FALSE;
@@ -781,7 +781,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return FALSE;
- name = vshCommandOptString(cmd, "devname", NULL);
+ vshCommandOptString(cmd, "devname", (char**) &name);
...or even worse, where you had to introduce a cast. Would you mind
splitting this into two patches?
The first patch should _just_ touch vshCommandOptString to change its
return type to const char *, and fix the fallout in the few functions
that need to be a bit more careful about type-safety. I already _know_
that cmdCd will be impacted: commit 5a814012 touched that function, and
exposed a place where we had been leaking memory when
vshCommandOptString returned NULL so we replaced things with a malloc'd
string, so there you will have to split things into using two variables,
one 'const char *' for vshCommandOptString, and the other 'char *' when
doing fallback. But hopefully most other functions will be okay with
using 'const char *' in the first place.
Then the second patch will swap argument order for all the vshCommandOpt
functions, including vshCommandOpt taking a const char ** third
argument, which should just work without any casts or removal of const
throughout the rest of the code.
That said,
@@ -1288,16 +1286,14 @@ static int
cmdDefine(vshControl *ctl, const vshCmd *cmd)
{
virDomainPtr dom;
- char *from;
- int found;
+ char *from = NULL;
int ret = TRUE;
char *buffer;
if (!vshConnectionUsability(ctl, ctl->conn))
return FALSE;
- from = vshCommandOptString(cmd, "file", &found);
- if (!found)
+ if (vshCommandOptString(cmd, "file", &from) <= 0)
return FALSE;
The rest of this patch (module the const issues) shows how nice the
change is! Your patch didn't include a diffstat, but it looks like you
have a net reduction in lines of code (always a good sign that the
cleanup was worth it).
@@ -2862,7 +2852,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd
*cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return FALSE;
- count = vshCommandOptInt(cmd, "count", &count);
+ vshCommandOptInt(cmd, "count", &count);
Why the indentation change here?
@@ -2918,7 +2908,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
{
virDomainPtr dom;
virDomainInfo info;
- unsigned long kilobytes;
+ unsigned long kilobytes = 0;
int ret = TRUE;
if (!vshConnectionUsability(ctl, ctl->conn))
@@ -2927,7 +2917,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return FALSE;
- kilobytes = vshCommandOptUL(cmd, "kilobytes", NULL);
+ vshCommandOptUL(cmd, "kilobytes", &kilobytes);
if (kilobytes <= 0) {
virDomainFree(dom);
vshError(ctl, _("Invalid value of %lu for memory size"), kilobytes);
Hmm, I wonder if we should have been checking if vshCommandOptUL
returned a negative value. Then again, since you defaulted the value to
0, and the value is unchanged on error, and an explicit 0 is also
invalid, you still end up with the right error message.
If you change the helper functions to be ATTRIBUTE_RETURN_CHECK, then
the compiler would enforce that you check all vshCommandOpt* functions
for parse error.
/*
- * Returns option as INT
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
+ * Convert option to int
+ * Return value:
+ * >0 if option found and valid (@value updated)
+ * 0 in case option not found (@value untouched)
+ * <0 in all other cases (@value untouched)
*/
static int
-vshCommandOptInt(const vshCmd *cmd, const char *name, int *found)
+vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
{
vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int res = 0, num_found = FALSE;
+ int ret = 0, num;
char *end_p = NULL;
if ((arg != NULL) && (arg->data != NULL)) {
- res = strtol(arg->data, &end_p, 10);
- if ((arg->data == end_p) || (*end_p!= 0))
- num_found = FALSE;
- else
- num_found = TRUE;
+ num = strtol(arg->data, &end_p, 10);
+ ret = -1;
+ if ((arg->data != end_p) && (*end_p == 0) && value) {
+ *value = num;
+ ret = 1;
+ }
}
- if (found)
- *found = num_found;
- return res;
+ return ret;
Nice! Exactly what I had in mind.
Looking forward to v3.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org