Re: [PATCH 3 of 4] xl: add support for vif rate limiting

["Mathieu Gagné" <mgagne@iweb.com>]

Forgot to reply to some comments. See below.

On 3/20/12 7:21 AM, Ian Campbell wrote:
> On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:
>
>> +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec)
>> +{
>> +    regex_t rec;
>> +    uint64_t tmp_bytes_per_sec = 0;
>> +
>> +    regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED);
>
> It seems that you use the regex only to check the syntax and then open
> code the parsing? That strikes me as odd, if you are going to use a
> regex parser you might as well use the matches returned from it.

I originally used the matches but faced a myriad of problems and 
challenges. I not only had one problem (this patch) but two (this patch 
and the regex/matches).

After much headache, I modified my approach to use regex for validation 
purpose only which simplified my code by a huge factor. IMO, the code 
was much more complex and difficult to read when I used matches.

I'm open to modify it if someone can help me with this.


> You could also combine the parsing of rate and interval into a single
> regex and avoid the use of strtok etc in the outermost function.

The original regex used sub-matches which I didn't know how or if it was 
possible to do in C:

^([0-9]+)([GMK]?)([Bb])/s(@([0-9]+)([mu]?)s)?$


> You should return a syntax error if the string is invalid.

The original code did not bailout on error and default to unlimited. 
Should we change this behavior in libxl/xl?


>> +        const char *p = bytes;
>> +        tmp_bytes_per_sec = strtoul(p, (char**)&p, 0);
>> +        if (*p == 'G' || *p == '\0')
>> +           tmp_bytes_per_sec *= 1000 * 1000 * 1000;
>> +        else if (*p == 'M')
>> +           tmp_bytes_per_sec *= 1000 * 1000;
>> +        else if (*p == 'K')
>> +           tmp_bytes_per_sec *= 1000;
>> +        if (*p == 'b' || *(p+1) == 'b')
>> +           tmp_bytes_per_sec /= 8;

Minor optimization to do here. It's impossible to fall in the second 
case of the first condition: *p == '\0'. I'll fix it in v2.

-- 
Mathieu

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