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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list