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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Jun 24, 2014 at 6:17 PM, George Dunlap
<george.dunlap@xxxxxxxxxxxxx> wrote:
> 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.

Dur... of course you only want to compare the first two characters,
not the null terminator.  N/m.

 -George

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