Re: [PATCH 07 of 10] xl: Fix 'script' param parsing in network-attach

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Sat, 2011-06-04 at 00:49 +0100, Marek Marczykowski wrote:
> On 03.06.2011 10:16, Ian Campbell wrote:
> > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1306963105 -7200
> >> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0
> >> # Parent  eb7216a75b7d7a5de93c717401b447545022b582
> >> xl: Fix 'script' param parsing in network-attach
> >>
> >> Fix 'script=' string length
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -4285,9 +4285,9 @@
> >>          } else if (!strncmp("ip=", *argv, 3)) {
> >>              free(nic.ip);
> >>              nic.ip = strdup((*argv) + 3);
> >> -        } else if (!strncmp("script=", *argv, 6)) {
> >> +        } else if (!strncmp("script=", *argv, 7)) {
> >>              free(nic.script);
> >> -            nic.script = strdup((*argv) + 6);
> >> +            nic.script = strdup((*argv) + 7);
> > 
> > Good catch.
> > 
> > The pre-existing use of all those
> > 	strncmp(A, *argv, open-coded-sizeof(A))
> > must be a source of many such errors. A helper function (or macro) is
> > probably the way to go. Do you fancy coding that up?
> 
> And the same size must be used in strdup offset...
> Maybe something like this (solving also problem in 06 patch):
> -----------
> #define COMPARE_AND_REPLACE(pattern, dest) \
>     } else if (!strncmp(pattern, *argv, sizeof(pattern))) { \
>         free(dest); \
>         dest = strdup((*argv) + sizeof(pattern));
> 
> (...)
>     COMPARE_AND_REPLACE("script=", nic.script)
>     COMPARE_AND_REPLACE("ip=", nic.ip)
> ---------
> Looks weird because of no semicolon at the end of lines, but should
> works.

Generally you can fix the semi-colon thing by omitting the last one from
the macro itself.

> Unfortunately cannot be used in all places (like one below)...
> What do you think?

How about (untested), requires a local "char *oparg":

static int match_option_size(const char *prefix, size_t len,
                             char *arg, char **argopt)
{
    int rc = strncmp(prefix, argv, len);
    if (!rc) *argopt = argv+len;
    return !rc
}
#define match_option(_prefix, _arg, _oparg) \
    match_option_size((_prefix), (_arg), sizeof((_prefix)), &(_oparg));

static void replace_string(char **str, const char *val)
{
    free(*str);\
    *str = strdup(val);
}

then you have:

    else if (match_option("script=", *argv, oparg))
        replace_string(&nic.script, oparg);
    else if (match_option("backend=", *argv, oparg)) {
        if(libxl_name_to_domid(&ctx, oparg, &val)) {
            fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
            ...
        }
    }

with a bit of macro pasting in match_option you can probably push the
"=" down into the macro so each callsite doesn't need it.

Ian.

> 
> > 
> > Ian.
> > 
> >>          } else if (!strncmp("backend=", *argv, 8)) {
> >>              if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
> >>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
> 
> 
>