Re: [Xen-devel] [PATCH v1 5/5] xentrace: Implement cpu mask range parsing of human values (-C).

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Jun 04, 2014 at 06:18:55PM +0100, George Dunlap wrote:
> On 06/04/2014 02:44 PM, Konrad Rzeszutek Wilk wrote:
> >Instead of just using -c 0x<some hex value> we can
> >also use -C <starting cpu>-<end cpu> or -C <cpu1>,<cpu2>
> 
> Would it be better, I wonder, to just try to overload the -c operator,
> special-casing "0x[hex]" to use the old interface?  Anyone who's currently
> using -c should almost certainly be using a hex string there, I should think
> -- using decimal would be pretty daft.
> 
> All it would take, I think, would be to check for bytes 0 and 1 being "0x",
> and if so, calling strtoul() rather than parse_cpumask_range().

Done.
> 
> [snip]
> >@@ -967,6 +970,98 @@ static int parse_cpumask(const char *arg)
> >      return 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 exp_digit, in_range;
> >+
> >+    if ( !buflen )
> >+    {
> >+        fprintf(stderr, "Invalid option argument: %s\n", arg);
> >+        usage(); /* does exit */
> >+    }
> >+    nmaskbits = xc_get_max_cpus(xc_handle);
> >+    if ( nmaskbits <= 0 )
> >+    {
> >+        fprintf(stderr, "Failed to get max number of CPUs! rc: %d\n", 
> >nmaskbits);
> >+        usage();
> >+    }
> >+    map = xc_cpumap_alloc(xc_handle);
> >+    if ( !map )
> >+    {
> >+        fprintf(stderr, "Out of memory!\n");
> >+        usage();
> >+    }
> >+    c = c_old = totaldigits = 0;
> >+    do {
> >+        exp_digit = 1;
> >+        in_range = 0;
> >+        a = b = 0;
> >+        while ( buflen )
> >+        {
> >+            c = *arg++;
> >+            buflen--;
> >+
> >+            if ( isspace(c) )
> >+                continue;
> 
> Is it possible for this to have a space at the beginning?  Doesn't getopt()
> take care of that?

Yes. If the user does something like this: -c "     0xff"

> 
> >+
> >+            if ( totaldigits && c && isspace(c_old) )
> 
> c_old doesn't seem to be set anywhere after it's initialized above.

Fixed
> 
> >+            {
> >+                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 ( exp_digit || in_range )
> >+                        goto err_out;
> 
> Isn't exp_digit a bit redundant, as if "in_range" is 1, "exp_digit" will
> also always be 1?

Fixed.
> 
> Everything else looks reasonable.

OK, posting a patch shortly
> 
>  -George
> 

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