[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.