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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-03-20 at 18:25 +0000, Mathieu Gagnà wrote:
> 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.

OK, the code does seem reasonably simple and you've written and tested
it so I'm inclined to suggest we go with it.

> > 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?

I think so.

While we strive to be compatible with xm in the common and valid cases I
think it is reasonable for xl to differ (i.e. improve) in the handling
of erroneous input or obscure corner cases.

Ian.

> >> +        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.
> 



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