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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org