Re: [PATCH v3 2/2] xentrace: Implement cpu mask range parsing of human values (-c).

[George Dunlap <george.dunlap@eu.citrix.com>]

On 06/20/2014 08:33 PM, Konrad Rzeszutek Wilk wrote:
> @@ -966,6 +969,129 @@ static int parse_cpumask(const char *arg)
>       return 0;
>   }
>
> +#define ZERO_DIGIT '0'
> +
> +static int parse_cpumask_range(const char *arg)
> +{
> +    xc_cpumap_t map;
> +    unsigned int a, b, buflen = strlen(arg);
> +    int c, c_old, totaldigits, nmaskbits;
> +    int in_range;
> +    const char *s;
> +
> +    if ( !buflen )
> +    {
> +        fprintf(stderr, "Invalid option argument: %s\n", arg);
> +        return EINVAL;
> +    }
> +    nmaskbits = xc_get_max_cpus(xc_handle);
> +    if ( nmaskbits <= 0 )
> +    {
> +        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", nmaskbits);
> +        return -ENOSPC;
> +    }
> +    map = xc_cpumap_alloc(xc_handle);
> +    if ( !map )
> +    {
> +        fprintf(stderr, "Out of memory!\n");
> +        return -ENOMEM;
> +    }
> +    c = c_old = totaldigits = 0;
> +    s = arg;
> +    do {
> +        in_range = 0;
> +        a = b = 0;
> +        while ( buflen )
> +        {
> +            c_old = c;
> +            c = *s++;
> +            buflen--;

Hmm, tricksy -- "buflen" may be non-zero above, but then may end up zero 
in the "} while()" below.  This caused me a bit of confusion -- might be 
worth a comment?

> +
> +            if ( isspace(c) )
> +                continue;
> +
> +            if ( totaldigits && c && isspace(c_old) )
> +            {
> +                fprintf(stderr, "No embedded whitespaces allowed in: %s\n", arg);
> +                goto err_out;
> +            }
> +
> +            /* A '\0' or a ',' signal the end of a cpu# or range */
> +            if ( c == '\0' || c == ',' )
> +                break;
> +
> +            if ( c == '-' )
> +            {
> +                if ( in_range )
> +                        goto err_out;
> +                b = 0;
> +                in_range = 1;

This would appear to accept both of the following:
  -c -5 # equivalent to 0-5
  -c 2,-6 # equivalent to 2, 0-6

Is that what you want?

If not, maybe in_range needs to be tristate: RANGE_INIT, RANGE_ONE, 
RANGE_TWO.

Alternately, you might consider accepting both "-N" as meaning "0-N", 
and "N-" as meaning "N-MAX_CPUS".  I think you could do that by adding 
"if(b==0) { b=nmaskbits-1; }" just after the inner loop.

The rest of the parsing here looks correct to me.

> +/**
> + * Figure out which of the CPU types the user has provided - either the hex
> + * variant or the cpu-list. Once done set the CPU mask.
> + */
> +static int figure_cpu_mask(void)
> +{
> +    int ret = 0;
> +    int buflen;
> +
> +    if ( opts.cpu_mask_str )
> +    {
> +        buflen = strlen(opts.cpu_mask_str);
> +        if ( buflen < 2 )

Isn't "-c 1" (i.e., just pinning to a single cpu) a valid option?

> +            goto out;
> +
> +        if ( strncmp("0x", opts.cpu_mask_str, 2) == 0 )

I think it should be safe to just to strcmp(), if one of your arguments 
is a static string, shouldn't it?  It's not that big a deal to me either 
way, though.

> +            ret = parse_cpumask(opts.cpu_mask_str);
> +        else
> +            ret = parse_cpumask_range(opts.cpu_mask_str);
> +    }
> +    if ( !ret )
> +        set_cpu_mask(opts.cpu_mask);

Having the "automatically set all bits" feature in set_cpu_mask() seems 
a bit confusing to me.  It also means that you have three distinct 
malloc() calls for the cpumask (one of which is wrong).

Would it make more sense to move the xc_cpumap_alloc() into this 
function, and then put the "automatically set all bits" as an else to 
the if(opts.cpu_mask_str) conditional?  That way you can see all the 
possibilities for what cpu_mask might end up in one place; and it 
automatically fixes the  (potential) bug with parse_cpumask() allocating 
a buffer that's too small.

Everything else looks good though.

  -George