Re: [Xen-devel] [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Sep 24, 2014 at 09:48:04PM +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@xxxxxxxxxx>
> ---
>  tools/libxl/xl_cmdimpl.c |   66 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1fc2171..d6f311f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -300,7 +300,6 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> -static char *xstrdup(const char *x) __attribute__ ((unused));
>  static char *xstrdup(const char *x)
>  {
>      char *r;
> @@ -564,6 +563,71 @@ static void split_string_into_string_list(const char 
> *str,
>      free(s);
>  }
>  
> +/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
> +   and look for CTYPE in libxl_internal.h */
> +typedef int (*char_predicate_t)(const int c);
> +
> +static void trim(char_predicate_t predicate, const char *input, char 
> **output) __attribute__ ((unused));
> +static void trim(char_predicate_t predicate, const char *input, char 
> **output)
> +{
> +    char *p, *q, *tmp;

Ideally you should check input != NULL before dereferencing it. Or you
need to document this function expects a valid pointer.

> +    if (*input == '\000')
> +        return;

As this function won't fail, caller cannot distinguish between a valid
pointer and garbage. I think you need to set output to NULL in this
case.

> +    /* Input has length >= 1 */
> +
> +    p = tmp = xstrdup(input);
> +    /* Skip past the first whitespace */

This comment is wrong.

> +    while ((*p != '\000') && (predicate((unsigned char)*p)))
> +        p ++;
> +    q = p + strlen(p) - 1;
> +    /* q points to the last non-NULL character */
> +    while ((q > p) && (predicate((unsigned char)*q)))
> +        q --;
> +    /* q points to the last character we want */
> +    q ++;
> +    *q = '\000';
> +    *output = xstrdup(p);
> +    free(tmp);
> +}
> +
> +static int split_string_into_pair(const char *str,
> +                                  const char *delim,
> +                                  char **a,
> +                                  char **b) __attribute__ ((unused));
> +static int split_string_into_pair(const char *str,
> +                                  const char *delim,
> +                                  char **a,
> +                                  char **b)
> +{

You didn't restrict number of entries in this function. So a malformed
"a=b=c" string ends up with "a" in key and "b=c" in value. Is that a
problem?

If value is to be passed directly to any other component (QEMU?) that
supports similar syntax as argument then I think we should be care about
this string.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel