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

[Mathieu Gagnà <mgagne@xxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel