xl: add support for vif rate limiting

Signed-off-by Mathieu Gagné <[email protected]>

tools/libxl/xl_cmdimpl.c +4 -4

@@ -841,10 +841,10 @@ static void parse_config_data(const char
                 nic->script = strdup(default_vifscript);
             }
 
-	    if (default_bridge) {
-		free(nic->bridge);
-		nic->bridge = strdup(default_bridge);
-	    }
+            if (default_bridge) {
+                free(nic->bridge);
+                nic->bridge = strdup(default_bridge);
+            }
 
             p = strtok(buf2, ",");
             if (!p)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Signed-off-by Mathieu Gagné <[email protected]>

tools/libxl/xl_cmdimpl.c +0 -1

@@ -841,10 +841,10 @@ static void parse_config_data(const char
                 nic->script = strdup(default_vifscript);
             }
 
-	    if (default_bridge) {

Add dryrun for testing and debugging purposes.

Signed-off-by Mathieu Gagné <[email protected]>

tools/libxl/xl_cmdimpl.c +10 -0

@@ -4756,6 +4756,16 @@ int main_networkattach(int argc, char **
             return 1;
         }
     }
+
+    if (dryrun_only) {
+        char *json = libxl_device_nic_to_json(ctx, &nic);
+        printf("vif: %s\n", json);
+        free(json);
+        libxl_device_nic_dispose(&nic);
+        if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
+        return 0;
+    }
+
     if (libxl_device_nic_add(ctx, domid, &nic)) {
         fprintf(stderr, "libxl_device_nic_add failed.\n");
         return 1;

tools/libxl/xl_cmdtable.c +1 -1

@@ -279,7 +279,7 @@ struct cmd_spec cmd_table[] = {
       "",
     },
     { "network-attach",
-      &main_networkattach, 0,
+      &main_networkattach, 1,
       "Create a new virtual network device",
       "<Domain> [type=<type>] [mac=<mac>] [bridge=<bridge>] "
       "[ip=<ip>] [script=<script>] [backend=<BackDomain>] [vifname=<name>] "

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Add dryrun for testing and debugging purposes.

The rate parameter specifies the rate at which the outgoing traffic will be limited to. This defaults to unlimited.

The rate keyword supports an optional time window parameter for specifying the granularity of credit replenishment. It determines the frequency at which the vif transmission credit is replenished. The default window is 50ms.

For example:

        'rate=10Mb/s'
        'rate=250KB/s'
        'rate=1MB/s@20ms'

Signed-off-by Mathieu Gagné <[email protected]>

docs/misc/xl-network-configuration.markdown +16 -0

@@ -122,5 +122,21 @@ Specifies the backend domain which this 
 defaults to domain 0. Specifying another domain requires setting up a
 driver domain which is outside the scope of this document.
 
+### rate
+
+Specifies the rate at which the outgoing traffic will be limited to. This
+defaults to unlimited.
+
+The `rate` keyword supports an optional time window parameter for specifying
+the granularity of credit replenishment. It determines the frequency at which
+the vif transmission credit is replenished. The default window is 50ms.
+
+For example:
+
+        'rate=10Mb/s'
+        'rate=250KB/s'
+        'rate=1MB/s@20ms'
+
+
 [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier
 [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking

tools/libxl/Makefile +1 -1

@@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 CLIENTS = xl testidl

tools/libxl/libxl.c +7 -0

@@ -1834,6 +1834,13 @@ int libxl_device_nic_add(libxl_ctx *ctx,
         flexarray_append(back, libxl__strdup(gc, nic->ip));
     }
 
+    if (nic->rate_interval_usecs > 0) {
+        flexarray_append(back, "rate");
+        flexarray_append(back, libxl__sprintf(gc, "%u,%u",
+                                              nic->rate_bytes_per_interval,
+                                              nic->rate_interval_usecs));
+    }
+
     flexarray_append(back, "bridge");
     flexarray_append(back, libxl__strdup(gc, nic->bridge));
     flexarray_append(back, "handle");

tools/libxl/libxl_types.idl +2 -0

@@ -341,6 +341,8 @@ libxl_device_nic = Struct("device_nic", 
     ("ifname", string),
     ("script", string),
     ("nictype", libxl_nic_type),
+    ("rate_bytes_per_interval", uint32),
+    ("rate_interval_usecs", uint32),
     ])
 
 libxl_device_pci = Struct("device_pci", [

tools/libxl/libxlu_internal.h +2 -0

@@ -17,9 +17,11 @@
 #define LIBXLU_INTERNAL_H
 
 #include <stdio.h>
+#include <stdlib.h>
 #include <errno.h>
 #include <string.h>
 #include <assert.h>
+#include <regex.h>
 
 #define XLU_ConfigList XLU_ConfigSetting
 

tools/libxl/libxlu_vif.c new file +73 -0

new file mode 100644
@@ -0,0 +1,91 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+
+static const char *vif_bytes_per_sec_re = "^([0-9]+)([GMK]?)([Bb])/s$";
+static const char *vif_internal_usec_re = "^([0-9]+)([mu]?)s?$";
+
+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);
+    if (regexec(&rec, bytes, 0, NULL, 0) == 0) {
+        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;
+    }
+    regfree(&rec);
+
+    if (tmp_bytes_per_sec > UINT32_MAX || tmp_bytes_per_sec == 0)
+        tmp_bytes_per_sec = UINT32_MAX;
+    *bytes_per_sec = tmp_bytes_per_sec;
+}
+
+static void vif_parse_rate_interval_usecs(const char *interval,
+                                          uint32_t *interval_usecs)
+{
+    regex_t rec;
+    uint64_t tmp_interval_usecs = 0;
+
+    regcomp(&rec, vif_internal_usec_re, REG_EXTENDED);
+    if (regexec(&rec, interval, 0, NULL, 0) == 0) {
+        const char *p = interval;
+        tmp_interval_usecs = strtoul(p, (char**)&p, 10);
+        if (*p == 's' || *p == '\0')
+            tmp_interval_usecs *= 1000 * 1000;
+        else if (*p == 'm')
+            tmp_interval_usecs *= 1000;
+    }
+    regfree(&rec);
+
+    if (tmp_interval_usecs > UINT32_MAX || tmp_interval_usecs == 0)
+        tmp_interval_usecs = UINT32_MAX;
+    *interval_usecs = tmp_interval_usecs;
+}
+
+void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval,
+                                          uint32_t *interval_usecs)
+{
+    uint32_t bytes_per_sec = 0;
+    uint64_t tmp_bytes_per_interval = 0;
+    char *tmprate, *ratetok;
+
+    tmprate = strdup(rate);
+
+    ratetok = strtok(tmprate, "@");
+    vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec);
+
+    ratetok = strtok(NULL, "@");
+    if (ratetok != NULL)
+        vif_parse_rate_interval_usecs(ratetok, interval_usecs);
+    else
+        *interval_usecs = 50000; /* Default to 50ms */
+
+    free(tmprate);
+

On Tue, 20 Mar 2012, Mathieu Gagné wrote:

Quoted 25 lines

tools/libxl/libxlu_vif.c new file +25 -0

+void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval,
+                                          uint32_t *interval_usecs)
+{
+    uint32_t bytes_per_sec = 0;
+    uint64_t tmp_bytes_per_interval = 0;
+    char *tmprate, *ratetok;
+
+    tmprate = strdup(rate);
+
+    ratetok = strtok(tmprate, "@");
+    vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec);
+
+    ratetok = strtok(NULL, "@");
+    if (ratetok != NULL)
+        vif_parse_rate_interval_usecs(ratetok, interval_usecs);
+    else
+        *interval_usecs = 50000; /* Default to 50ms */
+
+    free(tmprate);
+
+    tmp_bytes_per_interval =
+        (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L);
+
+    /* overflow checking: default to unlimited rate */
+    if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) {

tools/libxl/libxlu_vif.c line 83

Quoted 5 lines

tools/libxl/libxlu_vif.c new file +5 -0

+        tmp_bytes_per_interval = UINT32_MAX;
+        *interval_usecs = 0;
+    }
+    *bytes_per_interval = tmp_bytes_per_interval;
+}

wouldn't it make sense to just use a uint64_t for
rate_bytes_per_interval and rate_interval_usecs?

Quoted 3 lines · Stefano Stabellini

wouldn't it make sense to just use a uint64_t for
rate_bytes_per_interval and rate_interval_usecs?

It needn't be a barrier to this but FWIW Linux parses these into "unsigned long", but it does at least appear to do proper overflow checking.

Quoted 8 lines · Ian Campbell

Quoted 3 lines · Stefano Stabellini

wouldn't it make sense to just use a uint64_t for
rate_bytes_per_interval and rate_interval_usecs?

It needn't be a barrier to this but FWIW Linux parses these into "unsigned long", but it does at least appear to do proper overflow checking.

Xend/xm defaults to unlimited when bytes_per_interval or interval_usecs overflows a 32bit unsigned long.

Internally, xen-netback will parse those values into "unsigned long" which would otherwise overflow/wrap if we used uint64_t:

http://lxr.linux.no/#linux+v3.3/drivers/net/xen-netback/common.h#L86

The rate parameter specifies the rate at which the outgoing traffic will be limited to. This defaults to unlimited.

The rate keyword supports an optional time window parameter for specifying the granularity of credit replenishment. It determines the frequency at which the vif transmission credit is replenished. The default window is 50ms.

For example:

        'rate=10Mb/s'
        'rate=250KB/s'
        'rate=1MB/s@20ms'

Signed-off-by Mathieu Gagné <[email protected]>

docs/misc/xl-network-configuration.markdown +0 -0

@@ -122,5 +122,21 @@ Specifies the backend domain which this 
 defaults to domain 0. Specifying another domain requires setting up a

docs/misc/xl-network-configuration.markdown +2 -0

+
+
 [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier
 [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking

tools/libxl/Makefile +1 -1

@@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 CLIENTS = xl testidl

tools/libxl/libxl.c +7 -0

@@ -1834,6 +1834,13 @@ int libxl_device_nic_add(libxl_ctx *ctx,
         flexarray_append(back, libxl__strdup(gc, nic->ip));
     }
 
+    if (nic->rate_interval_usecs > 0) {
+        flexarray_append(back, "rate");
+        flexarray_append(back, libxl__sprintf(gc, "%u,%u",
+                                              nic->rate_bytes_per_interval,
+                                              nic->rate_interval_usecs));
+    }
+
     flexarray_append(back, "bridge");
     flexarray_append(back, libxl__strdup(gc, nic->bridge));
     flexarray_append(back, "handle");

tools/libxl/libxl_types.idl +2 -0

@@ -341,6 +341,8 @@ libxl_device_nic = Struct("device_nic", 
     ("ifname", string),
     ("script", string),
     ("nictype", libxl_nic_type),
+    ("rate_bytes_per_interval", uint32),
+    ("rate_interval_usecs", uint32),
     ])
 
 libxl_device_pci = Struct("device_pci", [

tools/libxl/libxlu_internal.h +2 -0

@@ -17,9 +17,11 @@
 #define LIBXLU_INTERNAL_H
 
 #include <stdio.h>
+#include <stdlib.h>
 #include <errno.h>
 #include <string.h>
 #include <assert.h>
+#include <regex.h>
 
 #define XLU_ConfigList XLU_ConfigSetting
 

tools/libxl/libxlu_vif.c new file +0 -0

new file mode 100644

tools/libxl/libxlu_vif.c new file +79 -0

+    if (regexec(&rec, bytes, 0, NULL, 0) == 0) {
+        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;
+    }
+    regfree(&rec);
+
+    if (tmp_bytes_per_sec > UINT32_MAX || tmp_bytes_per_sec == 0)
+        tmp_bytes_per_sec = UINT32_MAX;
+    *bytes_per_sec = tmp_bytes_per_sec;
+}
+
+static void vif_parse_rate_interval_usecs(const char *interval,
+                                          uint32_t *interval_usecs)
+{
+    regex_t rec;
+    uint64_t tmp_interval_usecs = 0;
+
+    regcomp(&rec, vif_internal_usec_re, REG_EXTENDED);
+    if (regexec(&rec, interval, 0, NULL, 0) == 0) {
+        const char *p = interval;
+        tmp_interval_usecs = strtoul(p, (char**)&p, 10);
+        if (*p == 's' || *p == '\0')
+            tmp_interval_usecs *= 1000 * 1000;
+        else if (*p == 'm')
+            tmp_interval_usecs *= 1000;
+    }
+    regfree(&rec);
+
+    if (tmp_interval_usecs > UINT32_MAX || tmp_interval_usecs == 0)
+        tmp_interval_usecs = UINT32_MAX;
+    *interval_usecs = tmp_interval_usecs;
+}
+
+void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval,
+                                          uint32_t *interval_usecs)
+{
+    uint32_t bytes_per_sec = 0;
+    uint64_t tmp_bytes_per_interval = 0;
+    char *tmprate, *ratetok;
+
+    tmprate = strdup(rate);
+
+    ratetok = strtok(tmprate, "@");
+    vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec);
+
+    ratetok = strtok(NULL, "@");
+    if (ratetok != NULL)
+        vif_parse_rate_interval_usecs(ratetok, interval_usecs);
+    else
+        *interval_usecs = 50000; /* Default to 50ms */
+
+    free(tmprate);
+
+    tmp_bytes_per_interval =
+        (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L);
+
+    /* overflow checking: default to unlimited rate */
+    if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) {
+        tmp_bytes_per_interval = UINT32_MAX;
+        *interval_usecs = 0;
+    }
+    *bytes_per_interval = tmp_bytes_per_interval;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

tools/libxl/libxlutil.h +6 -0

@@ -88,6 +88,12 @@ int xlu_disk_parse(XLU_Config *cfg, int 
    * resulting disk struct is used with libxl.
    */
 
+/*
+ * Vif rate parsing.
+ */
+
+void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval,
+                        uint32_t *interval_usecs);
 
 #endif /* LIBXLUTIL_H */
 

tools/libxl/xl_cmdimpl.c +4 -1

@@ -900,7 +900,8 @@ static void parse_config_data(const char
                         nic->backend_domid = 0;
                     }
                 } else if (!strcmp(p, "rate")) {
-                    fprintf(stderr, "the rate parameter for vifs is currently not supported\n");
+                    xlu_vif_parse_rate((p2 + 4), &(nic->rate_bytes_per_interval),
+                                                 &(nic->rate_interval_usecs));
                 } else if (!strcmp(p, "accel")) {
                     fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
                 }

@@ -4750,6 +4751,8 @@ int main_networkattach(int argc, char **
         } else if (MATCH_OPTION("model", *argv, oparg)) {
             replace_string(&nic.model, oparg);
         } else if (MATCH_OPTION("rate", *argv, oparg)) {
+            xlu_vif_parse_rate(oparg, &(nic.rate_bytes_per_interval),
+                                      &(nic.rate_interval_usecs));
         } else if (MATCH_OPTION("accel", *argv, oparg)) {
         } else {
             fprintf(stderr, "unrecognized argument `%s'\n", *argv);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

docs/misc/xl-network-configuration.markdown Specifies the backend domain which this · line 132

Quoted 9 lines · Mathieu Gagné

docs/misc/xl-network-configuration.markdown +8 -0

+### rate
+
+Specifies the rate at which the outgoing traffic will be limited to. This
+defaults to unlimited.
+
+The `rate` keyword supports an optional time window parameter for specifying
+the granularity of credit replenishment. It determines the frequency at which
+the vif transmission credit is replenished. The default window is 50ms.

I think this needs to be more explicit/formal about what the syntax is. e.g.:

The rate may be specified as "<RATE>/s" or optionally "<RATE>/s@<WINDOW>".

<RATE> is in bytes and can accept suffixes Mb, Kb (list here)

<WINDOW> is in microseconds and can accept suffixes<list>. The default is 50ms.

Is the "/s" formally required? Does it take any modifiers (e.g. ms)

Having both "/s" and "@<something>" in a single specification (e.g. "1MB/s@20ms") seems a bit odd, is that right? What does it actually mean

What are the semantics of window? If I say "1MB/s@500ms" then do I get 0.5MB of credit every half second?

The code and xend both seem to use "interval" rather than "window".

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+For example:

Should spell out the meaning of the examples:

Quoted 22 lines · Ian Campbell

docs/misc/xl-network-configuration.markdown Specifies the backend domain which this · line 132

Quoted 9 lines · Mathieu Gagné

docs/misc/xl-network-configuration.markdown +8 -0

+### rate
+
+Specifies the rate at which the outgoing traffic will be limited to. This
+defaults to unlimited.
+
+The `rate` keyword supports an optional time window parameter for specifying
+the granularity of credit replenishment. It determines the frequency at which
+the vif transmission credit is replenished. The default window is 50ms.

I think this needs to be more explicit/formal about what the syntax is. e.g.:

The rate may be specified as "<RATE>/s" or optionally "<RATE>/s@<WINDOW>".

<RATE> is in bytes and can accept suffixes Mb, Kb (list here)

<WINDOW> is in microseconds and can accept suffixes<list>. The default is 50ms.

Quoted 1 line

Is the "/s" formally required? Does it take any modifiers (e.g. ms)

It is formally required according to the original code within Xend/xm. Wanting to be 100% backward compatible, I did not change this part.

<RATE> corresponds to a rate and "/s" is required to correctly express a rate unit. I would therefore disagree about making it optional.

Should we add support for new modifiers? Would it make sense? Rate speed is mostly expressed in bits per second (bytes in our case), not us or ms. Would there be any use case?

Quoted 3 lines

Having both "/s" and "@<something>" in a single specification (e.g. "1MB/s@20ms") seems a bit odd, is that right? What does it actually mean

Vif rate limiting is credit-based. It means that for "1MB/s@20ms", the available credit will be equivalent of the traffic you would have done at "1MB/s" during 20ms. Your credit will be replenished every 20ms.

For your example:

   1MB/s => 1,000,000 bytes per sec
   20ms  => 20,000 us

This will results in a credit of 20,000 bytes replenished every 20,000 us.

Should I find a way to explain this concept in the documentation? English is not my mother tongue, I'll have difficulty to explain the concept is a clear and concise way.

Quoted 1 line

The code and xend both seem to use "interval" rather than "window".

The wording was retaken from the original commit [1] but I forgot to adjust it to its new context.

What would be the correct wording? The original patch used "window", xend used "interval", another proposed patch used "timeslice", "interval" and "time period".

tools/libxl/libxlu_vif.c line 12

Quoted 6 lines · Mathieu Gagné

tools/libxl/libxlu_vif.c new file +6 -0

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

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.

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

tools/libxl/libxlu_vif.c line 23

Quoted 10 lines

tools/libxl/libxlu_vif.c new file +10 -0

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

Forgot to reply to some comments. See below.

Quoted 12 lines · Ian Campbell

tools/libxl/libxlu_vif.c line 12

Quoted 6 lines · Mathieu Gagné

tools/libxl/libxlu_vif.c new file +6 -0

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

Quoted 2 lines

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

Quoted 1 line

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?

Quoted 10 lines

tools/libxl/libxlu_vif.c line 23

Quoted 10 lines

tools/libxl/libxlu_vif.c new file +10 -0

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

This test script runs "xl -N network-attach 0 <foobar>" against various rate syntax and checks that the output is as expected.

Signed-off-by Mathieu Gagné <[email protected]>

[...]+

tools/libxl/check-xl-vif-parse new file +9 -0

+# test invalid rate
+expected <<END
+vif: {
+    "backend_domid": 0,
+    "devid": 0,
+    "mtu": 0,
+    "model": null,
+    "mac": "00:00:00:00:00:00",
+    "ip": null,

tools/libxl/check-xl-vif-parse new file +18 -0

+one 0 rate=10MB
+one 0 rate=10MB/m
+one 0 rate=10ZB
+one 0 rate=10ZB/s
+one 0 rate=10ZB/m
+
+# test Kb/s unit
+expected <<END
+vif: {
+    "backend_domid": 0,
+    "devid": 0,
+    "mtu": 0,
+    "model": null,
+    "mac": "00:00:00:00:00:00",
+    "ip": null,
+    "bridge": null,
+    "ifname": null,
+    "script": null,

+[...]

tools/libxl/check-xl-vif-parse new file +12 -0

+# test bytes/s overflow
+expected <<END
+vif: {
+    "backend_domid": 0,
+    "devid": 0,
+    "mtu": 0,
+    "model": null,
+    "mac": "00:00:00:00:00:00",
+    "ip": null,
+    "bridge": null,
+    "ifname": null,
+    "script": null,

tools/libxl/check-xl-vif-parse new file +9 -0

+# test invalid interval
+expected <<END
+vif: {
+    "backend_domid": 0,
+    "devid": 0,
+    "mtu": 0,
+    "model": null,
+    "mac": "00:00:00:00:00:00",
+    "ip": null,

tools/libxl/check-xl-vif-parse new file +26 -0

+one 0 rate=10Mb/s@10h
+one 0 rate=10MB/s@foo
+one 0 rate=10MB/s@10h
+
+# test bytes/s with interval overflow
+expected <<END
+vif: {
+    "backend_domid": 0,
+    "devid": 0,
+    "mtu": 0,
+    "model": null,
+    "mac": "00:00:00:00:00:00",
+    "ip": null,
+    "bridge": null,
+    "ifname": null,
+    "script": null,
+    "nictype": null,
+    "rate_bytes_per_interval": 4294967295,
+    "rate_interval_usecs": 0
+}
+
+END
+one 0 rate=200Gb/s@10s
+one 0 rate=25GB/s@10s
+
+complete

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

This is my first patch to Xen. Any comments or feedbacks are welcomed and appreciated. Thanks!