[OPW PATCH V4] tools: xl: refactor code to parse network device options

[OPW PATCH V4] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu]

This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
adding parse_nic_config function. This function parses configuration
data and adds the information into libxl_device_nic struct. It is
called in both main_networkattach and parse_config_data functions
to replace duplicate code.

Signed-off-by: Alexandra Sandulescu <alecsandra.sandulescu@gmail.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 187 ++++++++++++++++++-----------------------------
 1 file changed, 70 insertions(+), 117 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ea43761..70de387 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -903,6 +903,73 @@ static void replace_string(char **str, const char *val)
     *str = xstrdup(val);
 }
 
+static int match_option_size(const char *prefix, size_t len,
+        char *arg, char **argopt)
+{
+    int rc = strncmp(prefix, arg, len);
+    if (!rc) *argopt = arg+len;
+    return !rc;
+}
+#define MATCH_OPTION(prefix, arg, oparg) \
+    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
+
+/* Parses network data and adds info into nic
+ * Returns 1 if the input token does not match one of the keys
+ * or parsed values are not correct. Successful parse returns 0 */
+static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token)
+{
+    char *endptr, *oparg;
+    int i;
+    unsigned int val;
+
+    if (MATCH_OPTION("type", token, oparg)) {
+        if (!strcmp("vif", oparg)) {
+            nic->nictype = LIBXL_NIC_TYPE_VIF;
+        } else if (!strcmp("ioemu", oparg)) {
+            nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
+        } else {
+            fprintf(stderr, "Invalid parameter `type'.\n");
+            return 1;
+        }
+    } else if (MATCH_OPTION("mac", token, oparg)) {
+        for (i = 0; i < 6; i++) {
+            val = strtoul(oparg, &endptr, 16);
+            if ((oparg == endptr) || (val > 255)) {
+                fprintf(stderr, "Invalid parameter `mac'.\n");
+                return 1;
+            }
+            nic->mac[i] = val;
+            oparg = endptr + 1;
+        }
+    } else if (MATCH_OPTION("bridge", token, oparg)) {
+        replace_string(&nic->bridge, oparg);
+    } else if (MATCH_OPTION("netdev", token, oparg)) {
+        fprintf(stderr, "the netdev parameter is deprecated, "
+                        "please use gatewaydev instead\n");
+        replace_string(&nic->gatewaydev, oparg);
+    } else if (MATCH_OPTION("gatewaydev", token, oparg)) {
+        replace_string(&nic->gatewaydev, oparg);
+    } else if (MATCH_OPTION("ip", token, oparg)) {
+        replace_string(&nic->ip, oparg);
+    } else if (MATCH_OPTION("script", token, oparg)) {
+        replace_string(&nic->script, oparg);
+    } else if (MATCH_OPTION("backend", token, oparg)) {
+        replace_string(&nic->backend_domname, oparg);
+    } else if (MATCH_OPTION("vifname", token, oparg)) {
+        replace_string(&nic->ifname, oparg);
+    } else if (MATCH_OPTION("model", token, oparg)) {
+        replace_string(&nic->model, oparg);
+    } else if (MATCH_OPTION("rate", token, oparg)) {
+        parse_vif_rate(config, oparg, nic);
+    } else if (MATCH_OPTION("accel", token, oparg)) {
+        fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
+    } else {
+        fprintf(stderr, "unrecognized argument `%s'\n", token);
+        return 1;
+    }
+    return 0;
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -1530,7 +1597,7 @@ static void parse_config_data(const char *config_source,
         while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics)) != NULL) {
             libxl_device_nic *nic;
             char *buf2 = strdup(buf);
-            char *p, *p2;
+            char *p;
 
             d_config->nics = (libxl_device_nic *) realloc(d_config->nics, sizeof (libxl_device_nic) * (d_config->num_nics+1));
             nic = d_config->nics + d_config->num_nics;
@@ -1544,64 +1611,7 @@ static void parse_config_data(const char *config_source,
             do {
                 while (*p == ' ')
                     p++;
-                if ((p2 = strchr(p, '=')) == NULL)
-                    break;
-                *p2 = '\0';
-                if (!strcmp(p, "model")) {
-                    free(nic->model);
-                    nic->model = strdup(p2 + 1);
-                } else if (!strcmp(p, "mac")) {
-                    char *p3 = p2 + 1;
-                    *(p3 + 2) = '\0';
-                    nic->mac[0] = strtol(p3, NULL, 16);
-                    p3 = p3 + 3;
-                    *(p3 + 2) = '\0';
-                    nic->mac[1] = strtol(p3, NULL, 16);
-                    p3 = p3 + 3;
-                    *(p3 + 2) = '\0';
-                    nic->mac[2] = strtol(p3, NULL, 16);
-                    p3 = p3 + 3;
-                    *(p3 + 2) = '\0';
-                    nic->mac[3] = strtol(p3, NULL, 16);
-                    p3 = p3 + 3;
-                    *(p3 + 2) = '\0';
-                    nic->mac[4] = strtol(p3, NULL, 16);
-                    p3 = p3 + 3;
-                    *(p3 + 2) = '\0';
-                    nic->mac[5] = strtol(p3, NULL, 16);
-                } else if (!strcmp(p, "bridge")) {
-                    free(nic->bridge);
-                    nic->bridge = strdup(p2 + 1);
-                } else if (!strcmp(p, "type")) {
-                    if (!strcmp(p2 + 1, "ioemu"))
-                        nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
-                    else
-                        nic->nictype = LIBXL_NIC_TYPE_VIF;
-                } else if (!strcmp(p, "ip")) {
-                    free(nic->ip);
-                    nic->ip = strdup(p2 + 1);
-                } else if (!strcmp(p, "script")) {
-                    free(nic->script);
-                    nic->script = strdup(p2 + 1);
-                } else if (!strcmp(p, "vifname")) {
-                    free(nic->ifname);
-                    nic->ifname = strdup(p2 + 1);
-                } else if (!strcmp(p, "backend")) {
-                    free(nic->backend_domname);
-                    nic->backend_domname = strdup(p2 + 1);
-                } else if (!strcmp(p, "rate")) {
-                    parse_vif_rate(&config, (p2 + 1), nic);
-                } else if (!strcmp(p, "accel")) {
-                    fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
-                } else if (!strcmp(p, "netdev")) {
-                    fprintf(stderr, "the netdev parameter is deprecated, "
-                                    "please use gatewaydev instead\n");
-                    free(nic->gatewaydev);
-                    nic->gatewaydev = strdup(p2 + 1);
-                } else if (!strcmp(p, "gatewaydev")) {
-                    free(nic->gatewaydev);
-                    nic->gatewaydev = strdup(p2 + 1);
-                }
+                parse_nic_config(nic, &config, p);
             } while ((p = strtok(NULL, ",")) != NULL);
 skip_nic:
             free(buf2);
@@ -2115,17 +2125,6 @@ static int handle_domain_death(uint32_t *r_domid,
     return restart;
 }
 
-/* for now used only by main_networkattach, but can be reused elsewhere */
-static int match_option_size(const char *prefix, size_t len,
-        char *arg, char **argopt)
-{
-    int rc = strncmp(prefix, arg, len);
-    if (!rc) *argopt = arg+len;
-    return !rc;
-}
-#define MATCH_OPTION(prefix, arg, oparg) \
-    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
-
 /* Preserve a copy of a domain under a new name. Updates *r_domid */
 static int preserve_domain(uint32_t *r_domid, libxl_event *event,
                            libxl_domain_config *d_config)
@@ -6138,10 +6137,6 @@ int main_networkattach(int argc, char **argv)
     int opt;
     libxl_device_nic nic;
     XLU_Config *config = 0;
-    char *endptr, *oparg;
-    const char *tok;
-    int i;
-    unsigned int val;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "network-attach", 1) {
         /* No options */
@@ -6164,50 +6159,8 @@ int main_networkattach(int argc, char **argv)
     set_default_nic_values(&nic);
 
     for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
-        if (MATCH_OPTION("type", *argv, oparg)) {
-            if (!strcmp("vif", oparg)) {
-                nic.nictype = LIBXL_NIC_TYPE_VIF;
-            } else if (!strcmp("ioemu", oparg)) {
-                nic.nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
-            } else {
-                fprintf(stderr, "Invalid parameter `type'.\n");
-                return 1;
-            }
-        } else if (MATCH_OPTION("mac", *argv, oparg)) {
-            tok = strtok(oparg, ":");
-            for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
-                val = strtoul(tok, &endptr, 16);
-                if ((tok == endptr) || (val > 255)) {
-                    fprintf(stderr, "Invalid parameter `mac'.\n");
-                    return 1;
-                }
-                nic.mac[i] = val;
-            }
-        } else if (MATCH_OPTION("bridge", *argv, oparg)) {
-            replace_string(&nic.bridge, oparg);
-        } else if (MATCH_OPTION("netdev", *argv, oparg)) {
-            fprintf(stderr, "the netdev parameter is deprecated, "
-                            "please use gatewaydev instead\n");
-            replace_string(&nic.gatewaydev, oparg);
-        } else if (MATCH_OPTION("gatewaydev", *argv, oparg)) {
-            replace_string(&nic.gatewaydev, oparg);
-        } else if (MATCH_OPTION("ip", *argv, oparg)) {
-            replace_string(&nic.ip, oparg);
-        } else if (MATCH_OPTION("script", *argv, oparg)) {
-            replace_string(&nic.script, oparg);
-        } else if (MATCH_OPTION("backend", *argv, oparg)) {
-            replace_string(&nic.backend_domname, oparg);
-        } else if (MATCH_OPTION("vifname", *argv, oparg)) {
-            replace_string(&nic.ifname, oparg);
-        } else if (MATCH_OPTION("model", *argv, oparg)) {
-            replace_string(&nic.model, oparg);
-        } else if (MATCH_OPTION("rate", *argv, oparg)) {
-            parse_vif_rate(&config, oparg, &nic);
-        } else if (MATCH_OPTION("accel", *argv, oparg)) {
-        } else {
-            fprintf(stderr, "unrecognized argument `%s'\n", *argv);
+        if (parse_nic_config(&nic, &config, *argv))
             return 1;
-        }
     }
 
     if (dryrun_only) {
-- 
1.9.1

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> adding parse_nic_config function. This function parses configuration
> data and adds the information into libxl_device_nic struct. It is
> called in both main_networkattach and parse_config_data functions
> to replace duplicate code.
> 
> Signed-off-by: Alexandra Sandulescu <alecsandra.sandulescu@gmail.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

This looks good to me, thanks. In reply to the first posting I asked:
Did you test both code paths? (wrt cfg file vs xl network-attach usage).
Did you?

Konrad, any reply to Wei's pros/cons on this patch for 4.5?
(<20141021152420.GI10234@zion.uk.xensource.com>)

Ian.

> ---
>  tools/libxl/xl_cmdimpl.c | 187 ++++++++++++++++++-----------------------------
>  1 file changed, 70 insertions(+), 117 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ea43761..70de387 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -903,6 +903,73 @@ static void replace_string(char **str, const char *val)
>      *str = xstrdup(val);
>  }
>  
> +static int match_option_size(const char *prefix, size_t len,
> +        char *arg, char **argopt)
> +{
> +    int rc = strncmp(prefix, arg, len);
> +    if (!rc) *argopt = arg+len;
> +    return !rc;
> +}
> +#define MATCH_OPTION(prefix, arg, oparg) \
> +    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
> +
> +/* Parses network data and adds info into nic
> + * Returns 1 if the input token does not match one of the keys
> + * or parsed values are not correct. Successful parse returns 0 */
> +static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token)
> +{
> +    char *endptr, *oparg;
> +    int i;
> +    unsigned int val;
> +
> +    if (MATCH_OPTION("type", token, oparg)) {
> +        if (!strcmp("vif", oparg)) {
> +            nic->nictype = LIBXL_NIC_TYPE_VIF;
> +        } else if (!strcmp("ioemu", oparg)) {
> +            nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> +        } else {
> +            fprintf(stderr, "Invalid parameter `type'.\n");
> +            return 1;
> +        }
> +    } else if (MATCH_OPTION("mac", token, oparg)) {
> +        for (i = 0; i < 6; i++) {
> +            val = strtoul(oparg, &endptr, 16);
> +            if ((oparg == endptr) || (val > 255)) {
> +                fprintf(stderr, "Invalid parameter `mac'.\n");
> +                return 1;
> +            }
> +            nic->mac[i] = val;
> +            oparg = endptr + 1;
> +        }
> +    } else if (MATCH_OPTION("bridge", token, oparg)) {
> +        replace_string(&nic->bridge, oparg);
> +    } else if (MATCH_OPTION("netdev", token, oparg)) {
> +        fprintf(stderr, "the netdev parameter is deprecated, "
> +                        "please use gatewaydev instead\n");
> +        replace_string(&nic->gatewaydev, oparg);
> +    } else if (MATCH_OPTION("gatewaydev", token, oparg)) {
> +        replace_string(&nic->gatewaydev, oparg);
> +    } else if (MATCH_OPTION("ip", token, oparg)) {
> +        replace_string(&nic->ip, oparg);
> +    } else if (MATCH_OPTION("script", token, oparg)) {
> +        replace_string(&nic->script, oparg);
> +    } else if (MATCH_OPTION("backend", token, oparg)) {
> +        replace_string(&nic->backend_domname, oparg);
> +    } else if (MATCH_OPTION("vifname", token, oparg)) {
> +        replace_string(&nic->ifname, oparg);
> +    } else if (MATCH_OPTION("model", token, oparg)) {
> +        replace_string(&nic->model, oparg);
> +    } else if (MATCH_OPTION("rate", token, oparg)) {
> +        parse_vif_rate(config, oparg, nic);
> +    } else if (MATCH_OPTION("accel", token, oparg)) {
> +        fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
> +    } else {
> +        fprintf(stderr, "unrecognized argument `%s'\n", token);
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  static void parse_config_data(const char *config_source,
>                                const char *config_data,
>                                int config_len,
> @@ -1530,7 +1597,7 @@ static void parse_config_data(const char *config_source,
>          while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics)) != NULL) {
>              libxl_device_nic *nic;
>              char *buf2 = strdup(buf);
> -            char *p, *p2;
> +            char *p;
>  
>              d_config->nics = (libxl_device_nic *) realloc(d_config->nics, sizeof (libxl_device_nic) * (d_config->num_nics+1));
>              nic = d_config->nics + d_config->num_nics;
> @@ -1544,64 +1611,7 @@ static void parse_config_data(const char *config_source,
>              do {
>                  while (*p == ' ')
>                      p++;
> -                if ((p2 = strchr(p, '=')) == NULL)
> -                    break;
> -                *p2 = '\0';
> -                if (!strcmp(p, "model")) {
> -                    free(nic->model);
> -                    nic->model = strdup(p2 + 1);
> -                } else if (!strcmp(p, "mac")) {
> -                    char *p3 = p2 + 1;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[0] = strtol(p3, NULL, 16);
> -                    p3 = p3 + 3;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[1] = strtol(p3, NULL, 16);
> -                    p3 = p3 + 3;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[2] = strtol(p3, NULL, 16);
> -                    p3 = p3 + 3;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[3] = strtol(p3, NULL, 16);
> -                    p3 = p3 + 3;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[4] = strtol(p3, NULL, 16);
> -                    p3 = p3 + 3;
> -                    *(p3 + 2) = '\0';
> -                    nic->mac[5] = strtol(p3, NULL, 16);
> -                } else if (!strcmp(p, "bridge")) {
> -                    free(nic->bridge);
> -                    nic->bridge = strdup(p2 + 1);
> -                } else if (!strcmp(p, "type")) {
> -                    if (!strcmp(p2 + 1, "ioemu"))
> -                        nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> -                    else
> -                        nic->nictype = LIBXL_NIC_TYPE_VIF;
> -                } else if (!strcmp(p, "ip")) {
> -                    free(nic->ip);
> -                    nic->ip = strdup(p2 + 1);
> -                } else if (!strcmp(p, "script")) {
> -                    free(nic->script);
> -                    nic->script = strdup(p2 + 1);
> -                } else if (!strcmp(p, "vifname")) {
> -                    free(nic->ifname);
> -                    nic->ifname = strdup(p2 + 1);
> -                } else if (!strcmp(p, "backend")) {
> -                    free(nic->backend_domname);
> -                    nic->backend_domname = strdup(p2 + 1);
> -                } else if (!strcmp(p, "rate")) {
> -                    parse_vif_rate(&config, (p2 + 1), nic);
> -                } else if (!strcmp(p, "accel")) {
> -                    fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
> -                } else if (!strcmp(p, "netdev")) {
> -                    fprintf(stderr, "the netdev parameter is deprecated, "
> -                                    "please use gatewaydev instead\n");
> -                    free(nic->gatewaydev);
> -                    nic->gatewaydev = strdup(p2 + 1);
> -                } else if (!strcmp(p, "gatewaydev")) {
> -                    free(nic->gatewaydev);
> -                    nic->gatewaydev = strdup(p2 + 1);
> -                }
> +                parse_nic_config(nic, &config, p);
>              } while ((p = strtok(NULL, ",")) != NULL);
>  skip_nic:
>              free(buf2);
> @@ -2115,17 +2125,6 @@ static int handle_domain_death(uint32_t *r_domid,
>      return restart;
>  }
>  
> -/* for now used only by main_networkattach, but can be reused elsewhere */
> -static int match_option_size(const char *prefix, size_t len,
> -        char *arg, char **argopt)
> -{
> -    int rc = strncmp(prefix, arg, len);
> -    if (!rc) *argopt = arg+len;
> -    return !rc;
> -}
> -#define MATCH_OPTION(prefix, arg, oparg) \
> -    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
> -
>  /* Preserve a copy of a domain under a new name. Updates *r_domid */
>  static int preserve_domain(uint32_t *r_domid, libxl_event *event,
>                             libxl_domain_config *d_config)
> @@ -6138,10 +6137,6 @@ int main_networkattach(int argc, char **argv)
>      int opt;
>      libxl_device_nic nic;
>      XLU_Config *config = 0;
> -    char *endptr, *oparg;
> -    const char *tok;
> -    int i;
> -    unsigned int val;
>  
>      SWITCH_FOREACH_OPT(opt, "", NULL, "network-attach", 1) {
>          /* No options */
> @@ -6164,50 +6159,8 @@ int main_networkattach(int argc, char **argv)
>      set_default_nic_values(&nic);
>  
>      for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
> -        if (MATCH_OPTION("type", *argv, oparg)) {
> -            if (!strcmp("vif", oparg)) {
> -                nic.nictype = LIBXL_NIC_TYPE_VIF;
> -            } else if (!strcmp("ioemu", oparg)) {
> -                nic.nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> -            } else {
> -                fprintf(stderr, "Invalid parameter `type'.\n");
> -                return 1;
> -            }
> -        } else if (MATCH_OPTION("mac", *argv, oparg)) {
> -            tok = strtok(oparg, ":");
> -            for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
> -                val = strtoul(tok, &endptr, 16);
> -                if ((tok == endptr) || (val > 255)) {
> -                    fprintf(stderr, "Invalid parameter `mac'.\n");
> -                    return 1;
> -                }
> -                nic.mac[i] = val;
> -            }
> -        } else if (MATCH_OPTION("bridge", *argv, oparg)) {
> -            replace_string(&nic.bridge, oparg);
> -        } else if (MATCH_OPTION("netdev", *argv, oparg)) {
> -            fprintf(stderr, "the netdev parameter is deprecated, "
> -                            "please use gatewaydev instead\n");
> -            replace_string(&nic.gatewaydev, oparg);
> -        } else if (MATCH_OPTION("gatewaydev", *argv, oparg)) {
> -            replace_string(&nic.gatewaydev, oparg);
> -        } else if (MATCH_OPTION("ip", *argv, oparg)) {
> -            replace_string(&nic.ip, oparg);
> -        } else if (MATCH_OPTION("script", *argv, oparg)) {
> -            replace_string(&nic.script, oparg);
> -        } else if (MATCH_OPTION("backend", *argv, oparg)) {
> -            replace_string(&nic.backend_domname, oparg);
> -        } else if (MATCH_OPTION("vifname", *argv, oparg)) {
> -            replace_string(&nic.ifname, oparg);
> -        } else if (MATCH_OPTION("model", *argv, oparg)) {
> -            replace_string(&nic.model, oparg);
> -        } else if (MATCH_OPTION("rate", *argv, oparg)) {
> -            parse_vif_rate(&config, oparg, &nic);
> -        } else if (MATCH_OPTION("accel", *argv, oparg)) {
> -        } else {
> -            fprintf(stderr, "unrecognized argument `%s'\n", *argv);
> +        if (parse_nic_config(&nic, &config, *argv))
>              return 1;
> -        }
>      }
>  
>      if (dryrun_only) {

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Konrad Rzeszutek Wilk]

On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
> On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> > adding parse_nic_config function. This function parses configuration
> > data and adds the information into libxl_device_nic struct. It is
> > called in both main_networkattach and parse_config_data functions
> > to replace duplicate code.
> > 
> > Signed-off-by: Alexandra Sandulescu <alecsandra.sandulescu@gmail.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> This looks good to me, thanks. In reply to the first posting I asked:
> Did you test both code paths? (wrt cfg file vs xl network-attach usage).
> Did you?
> 
> Konrad, any reply to Wei's pros/cons on this patch for 4.5?
> (<20141021152420.GI10234@zion.uk.xensource.com>)

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I am making this based on the fact that:
 - It has run through the OSSTest which does a ton of tests so the
   chance of regression is almost nill.

 - It is a nice cleanup and adds more to the: more deletion than
   addition campaign :-)

 - It does not add any new code, just shuffles code around so that
   is good.

 - The tests that Ian mentioned do not show any regressions.

Thank you.
> 
> Ian.
> 
> > ---
> >  tools/libxl/xl_cmdimpl.c | 187 ++++++++++++++++++-----------------------------
> >  1 file changed, 70 insertions(+), 117 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index ea43761..70de387 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -903,6 +903,73 @@ static void replace_string(char **str, const char *val)
> >      *str = xstrdup(val);
> >  }
> >  
> > +static int match_option_size(const char *prefix, size_t len,
> > +        char *arg, char **argopt)
> > +{
> > +    int rc = strncmp(prefix, arg, len);
> > +    if (!rc) *argopt = arg+len;
> > +    return !rc;
> > +}
> > +#define MATCH_OPTION(prefix, arg, oparg) \
> > +    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
> > +
> > +/* Parses network data and adds info into nic
> > + * Returns 1 if the input token does not match one of the keys
> > + * or parsed values are not correct. Successful parse returns 0 */
> > +static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token)
> > +{
> > +    char *endptr, *oparg;
> > +    int i;
> > +    unsigned int val;
> > +
> > +    if (MATCH_OPTION("type", token, oparg)) {
> > +        if (!strcmp("vif", oparg)) {
> > +            nic->nictype = LIBXL_NIC_TYPE_VIF;
> > +        } else if (!strcmp("ioemu", oparg)) {
> > +            nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> > +        } else {
> > +            fprintf(stderr, "Invalid parameter `type'.\n");
> > +            return 1;
> > +        }
> > +    } else if (MATCH_OPTION("mac", token, oparg)) {
> > +        for (i = 0; i < 6; i++) {
> > +            val = strtoul(oparg, &endptr, 16);
> > +            if ((oparg == endptr) || (val > 255)) {
> > +                fprintf(stderr, "Invalid parameter `mac'.\n");
> > +                return 1;
> > +            }
> > +            nic->mac[i] = val;
> > +            oparg = endptr + 1;
> > +        }
> > +    } else if (MATCH_OPTION("bridge", token, oparg)) {
> > +        replace_string(&nic->bridge, oparg);
> > +    } else if (MATCH_OPTION("netdev", token, oparg)) {
> > +        fprintf(stderr, "the netdev parameter is deprecated, "
> > +                        "please use gatewaydev instead\n");
> > +        replace_string(&nic->gatewaydev, oparg);
> > +    } else if (MATCH_OPTION("gatewaydev", token, oparg)) {
> > +        replace_string(&nic->gatewaydev, oparg);
> > +    } else if (MATCH_OPTION("ip", token, oparg)) {
> > +        replace_string(&nic->ip, oparg);
> > +    } else if (MATCH_OPTION("script", token, oparg)) {
> > +        replace_string(&nic->script, oparg);
> > +    } else if (MATCH_OPTION("backend", token, oparg)) {
> > +        replace_string(&nic->backend_domname, oparg);
> > +    } else if (MATCH_OPTION("vifname", token, oparg)) {
> > +        replace_string(&nic->ifname, oparg);
> > +    } else if (MATCH_OPTION("model", token, oparg)) {
> > +        replace_string(&nic->model, oparg);
> > +    } else if (MATCH_OPTION("rate", token, oparg)) {
> > +        parse_vif_rate(config, oparg, nic);
> > +    } else if (MATCH_OPTION("accel", token, oparg)) {
> > +        fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
> > +    } else {
> > +        fprintf(stderr, "unrecognized argument `%s'\n", token);
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> >  static void parse_config_data(const char *config_source,
> >                                const char *config_data,
> >                                int config_len,
> > @@ -1530,7 +1597,7 @@ static void parse_config_data(const char *config_source,
> >          while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics)) != NULL) {
> >              libxl_device_nic *nic;
> >              char *buf2 = strdup(buf);
> > -            char *p, *p2;
> > +            char *p;
> >  
> >              d_config->nics = (libxl_device_nic *) realloc(d_config->nics, sizeof (libxl_device_nic) * (d_config->num_nics+1));
> >              nic = d_config->nics + d_config->num_nics;
> > @@ -1544,64 +1611,7 @@ static void parse_config_data(const char *config_source,
> >              do {
> >                  while (*p == ' ')
> >                      p++;
> > -                if ((p2 = strchr(p, '=')) == NULL)
> > -                    break;
> > -                *p2 = '\0';
> > -                if (!strcmp(p, "model")) {
> > -                    free(nic->model);
> > -                    nic->model = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "mac")) {
> > -                    char *p3 = p2 + 1;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[0] = strtol(p3, NULL, 16);
> > -                    p3 = p3 + 3;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[1] = strtol(p3, NULL, 16);
> > -                    p3 = p3 + 3;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[2] = strtol(p3, NULL, 16);
> > -                    p3 = p3 + 3;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[3] = strtol(p3, NULL, 16);
> > -                    p3 = p3 + 3;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[4] = strtol(p3, NULL, 16);
> > -                    p3 = p3 + 3;
> > -                    *(p3 + 2) = '\0';
> > -                    nic->mac[5] = strtol(p3, NULL, 16);
> > -                } else if (!strcmp(p, "bridge")) {
> > -                    free(nic->bridge);
> > -                    nic->bridge = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "type")) {
> > -                    if (!strcmp(p2 + 1, "ioemu"))
> > -                        nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> > -                    else
> > -                        nic->nictype = LIBXL_NIC_TYPE_VIF;
> > -                } else if (!strcmp(p, "ip")) {
> > -                    free(nic->ip);
> > -                    nic->ip = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "script")) {
> > -                    free(nic->script);
> > -                    nic->script = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "vifname")) {
> > -                    free(nic->ifname);
> > -                    nic->ifname = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "backend")) {
> > -                    free(nic->backend_domname);
> > -                    nic->backend_domname = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "rate")) {
> > -                    parse_vif_rate(&config, (p2 + 1), nic);
> > -                } else if (!strcmp(p, "accel")) {
> > -                    fprintf(stderr, "the accel parameter for vifs is currently not supported\n");
> > -                } else if (!strcmp(p, "netdev")) {
> > -                    fprintf(stderr, "the netdev parameter is deprecated, "
> > -                                    "please use gatewaydev instead\n");
> > -                    free(nic->gatewaydev);
> > -                    nic->gatewaydev = strdup(p2 + 1);
> > -                } else if (!strcmp(p, "gatewaydev")) {
> > -                    free(nic->gatewaydev);
> > -                    nic->gatewaydev = strdup(p2 + 1);
> > -                }
> > +                parse_nic_config(nic, &config, p);
> >              } while ((p = strtok(NULL, ",")) != NULL);
> >  skip_nic:
> >              free(buf2);
> > @@ -2115,17 +2125,6 @@ static int handle_domain_death(uint32_t *r_domid,
> >      return restart;
> >  }
> >  
> > -/* for now used only by main_networkattach, but can be reused elsewhere */
> > -static int match_option_size(const char *prefix, size_t len,
> > -        char *arg, char **argopt)
> > -{
> > -    int rc = strncmp(prefix, arg, len);
> > -    if (!rc) *argopt = arg+len;
> > -    return !rc;
> > -}
> > -#define MATCH_OPTION(prefix, arg, oparg) \
> > -    match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
> > -
> >  /* Preserve a copy of a domain under a new name. Updates *r_domid */
> >  static int preserve_domain(uint32_t *r_domid, libxl_event *event,
> >                             libxl_domain_config *d_config)
> > @@ -6138,10 +6137,6 @@ int main_networkattach(int argc, char **argv)
> >      int opt;
> >      libxl_device_nic nic;
> >      XLU_Config *config = 0;
> > -    char *endptr, *oparg;
> > -    const char *tok;
> > -    int i;
> > -    unsigned int val;
> >  
> >      SWITCH_FOREACH_OPT(opt, "", NULL, "network-attach", 1) {
> >          /* No options */
> > @@ -6164,50 +6159,8 @@ int main_networkattach(int argc, char **argv)
> >      set_default_nic_values(&nic);
> >  
> >      for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
> > -        if (MATCH_OPTION("type", *argv, oparg)) {
> > -            if (!strcmp("vif", oparg)) {
> > -                nic.nictype = LIBXL_NIC_TYPE_VIF;
> > -            } else if (!strcmp("ioemu", oparg)) {
> > -                nic.nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> > -            } else {
> > -                fprintf(stderr, "Invalid parameter `type'.\n");
> > -                return 1;
> > -            }
> > -        } else if (MATCH_OPTION("mac", *argv, oparg)) {
> > -            tok = strtok(oparg, ":");
> > -            for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
> > -                val = strtoul(tok, &endptr, 16);
> > -                if ((tok == endptr) || (val > 255)) {
> > -                    fprintf(stderr, "Invalid parameter `mac'.\n");
> > -                    return 1;
> > -                }
> > -                nic.mac[i] = val;
> > -            }
> > -        } else if (MATCH_OPTION("bridge", *argv, oparg)) {
> > -            replace_string(&nic.bridge, oparg);
> > -        } else if (MATCH_OPTION("netdev", *argv, oparg)) {
> > -            fprintf(stderr, "the netdev parameter is deprecated, "
> > -                            "please use gatewaydev instead\n");
> > -            replace_string(&nic.gatewaydev, oparg);
> > -        } else if (MATCH_OPTION("gatewaydev", *argv, oparg)) {
> > -            replace_string(&nic.gatewaydev, oparg);
> > -        } else if (MATCH_OPTION("ip", *argv, oparg)) {
> > -            replace_string(&nic.ip, oparg);
> > -        } else if (MATCH_OPTION("script", *argv, oparg)) {
> > -            replace_string(&nic.script, oparg);
> > -        } else if (MATCH_OPTION("backend", *argv, oparg)) {
> > -            replace_string(&nic.backend_domname, oparg);
> > -        } else if (MATCH_OPTION("vifname", *argv, oparg)) {
> > -            replace_string(&nic.ifname, oparg);
> > -        } else if (MATCH_OPTION("model", *argv, oparg)) {
> > -            replace_string(&nic.model, oparg);
> > -        } else if (MATCH_OPTION("rate", *argv, oparg)) {
> > -            parse_vif_rate(&config, oparg, &nic);
> > -        } else if (MATCH_OPTION("accel", *argv, oparg)) {
> > -        } else {
> > -            fprintf(stderr, "unrecognized argument `%s'\n", *argv);
> > +        if (parse_nic_config(&nic, &config, *argv))
> >              return 1;
> > -        }
> >      }
> >  
> >      if (dryrun_only) {
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Wed, 2014-10-22 at 16:21 -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
> > On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> > > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> > > adding parse_nic_config function. This function parses configuration
> > > data and adds the information into libxl_device_nic struct. It is
> > > called in both main_networkattach and parse_config_data functions
> > > to replace duplicate code.
> > > 
> > > Signed-off-by: Alexandra Sandulescu <alecsandra.sandulescu@gmail.com>
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > This looks good to me, thanks. In reply to the first posting I asked:
> > Did you test both code paths? (wrt cfg file vs xl network-attach usage).
> > Did you?
> > 
> > Konrad, any reply to Wei's pros/cons on this patch for 4.5?
> > (<20141021152420.GI10234@zion.uk.xensource.com>)
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I am making this based on the fact that:
>  - It has run through the OSSTest which does a ton of tests so the
>    chance of regression is almost nill.

You mean "will", not "has", right? Since it won't be run through osstest
until it is committed.

Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Konrad Rzeszutek Wilk]

On October 23, 2014 3:56:14 AM EDT, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>On Wed, 2014-10-22 at 16:21 -0400, Konrad Rzeszutek Wilk wrote:
>> On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
>> > On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
>> > > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
>> > > adding parse_nic_config function. This function parses
>configuration
>> > > data and adds the information into libxl_device_nic struct. It is
>> > > called in both main_networkattach and parse_config_data functions
>> > > to replace duplicate code.
>> > > 
>> > > Signed-off-by: Alexandra Sandulescu
><alecsandra.sandulescu@gmail.com>
>> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
>> > 
>> > This looks good to me, thanks. In reply to the first posting I
>asked:
>> > Did you test both code paths? (wrt cfg file vs xl network-attach
>usage).
>> > Did you?
>> > 
>> > Konrad, any reply to Wei's pros/cons on this patch for 4.5?
>> > (<20141021152420.GI10234@zion.uk.xensource.com>)
>> 
>> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> 
>> I am making this based on the fact that:
>>  - It has run through the OSSTest which does a ton of tests so the
>>    chance of regression is almost nill.
>
>You mean "will", not "has", right? Since it won't be run through
>osstest
>until it is committed.

Has. Wei said it had run through it.

>
>Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Thu, 2014-10-23 at 16:08 -0400, Konrad Rzeszutek Wilk wrote:
> On October 23, 2014 3:56:14 AM EDT, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >On Wed, 2014-10-22 at 16:21 -0400, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
> >> > On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> >> > > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> >> > > adding parse_nic_config function. This function parses
> >configuration
> >> > > data and adds the information into libxl_device_nic struct. It is
> >> > > called in both main_networkattach and parse_config_data functions
> >> > > to replace duplicate code.
> >> > > 
> >> > > Signed-off-by: Alexandra Sandulescu
> ><alecsandra.sandulescu@gmail.com>
> >> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >> > 
> >> > This looks good to me, thanks. In reply to the first posting I
> >asked:
> >> > Did you test both code paths? (wrt cfg file vs xl network-attach
> >usage).
> >> > Did you?
> >> > 
> >> > Konrad, any reply to Wei's pros/cons on this patch for 4.5?
> >> > (<20141021152420.GI10234@zion.uk.xensource.com>)
> >> 
> >> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> 
> >> I am making this based on the fact that:
> >>  - It has run through the OSSTest which does a ton of tests so the
> >>    chance of regression is almost nill.
> >
> >You mean "will", not "has", right? Since it won't be run through
> >osstest
> >until it is committed.
> 
> Has. Wei said it had run through it.

Do you mean when he said "2) the code path is tested in OSSTest." ?

He said the code *path*, not the code, IOW I would read that as "would
be tested", NOT "has been tested".

Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Konrad Rzeszutek Wilk]

[-- Attachment #1.1: Type: text/plain, Size: 2070 bytes --]

On Oct 23, 2014 6:42 PM, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
>
> On Thu, 2014-10-23 at 16:08 -0400, Konrad Rzeszutek Wilk wrote:
> > On October 23, 2014 3:56:14 AM EDT, Ian Campbell <
Ian.Campbell@citrix.com> wrote:
> > >On Wed, 2014-10-22 at 16:21 -0400, Konrad Rzeszutek Wilk wrote:
> > >> On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
> > >> > On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> > >> > > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> > >> > > adding parse_nic_config function. This function parses
> > >configuration
> > >> > > data and adds the information into libxl_device_nic struct. It is
> > >> > > called in both main_networkattach and parse_config_data functions
> > >> > > to replace duplicate code.
> > >> > >
> > >> > > Signed-off-by: Alexandra Sandulescu
> > ><alecsandra.sandulescu@gmail.com>
> > >> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > >> >
> > >> > This looks good to me, thanks. In reply to the first posting I
> > >asked:
> > >> > Did you test both code paths? (wrt cfg file vs xl network-attach
> > >usage).
> > >> > Did you?
> > >> >
> > >> > Konrad, any reply to Wei's pros/cons on this patch for 4.5?
> > >> > (<20141021152420.GI10234@zion.uk.xensource.com>)
> > >>
> > >> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >>
> > >> I am making this based on the fact that:
> > >>  - It has run through the OSSTest which does a ton of tests so the
> > >>    chance of regression is almost nill.
> > >
> > >You mean "will", not "has", right? Since it won't be run through
> > >osstest
> > >until it is committed.
> >
> > Has. Wei said it had run through it.
>
> Do you mean when he said "2) the code path is tested in OSSTest." ?
>

Yes.

> He said the code *path*, not the code, IOW I would read that as "would
> be tested", NOT "has been tested".
>

Aye. In which case let's wait for Alexandra's response to the testing that
Wei asked for.

On a related note - one can setup OSSTest on ones own network, fairly easy
right?

> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 3293 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Thu, 2014-10-23 at 19:08 -0400, Konrad Rzeszutek Wilk wrote:
> > He said the code *path*, not the code, IOW I would read that as "would
> > be tested", NOT "has been tested".
> >
> 
> Aye. In which case let's wait for Alexandra's response to the testing
> that Wei asked for.

Alexandra replied to me (privately) a couple of days ago confirming that
she has tested both code paths.

> On a related note - one can setup OSSTest on ones own network, fairly easy right?

Right in "standalone mode". It's far easier than it once was and we've
been squashing speedbumps as new people try it.

You do need some infrastructure such as PXE and dhcp and at least two
machines (controller + test host). I think it would be overkill for a
change of this magnitude (if that's what you were driving at).

I think
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;h=9a85549cb600fa5be91bcbae6605f78616db63da;hb=2aa4fe7a4b138e5e88387af66f4fe14039a1f7ea is the best starting point for osstest, including standalone mode operation.

Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu]

[-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --]

Hello,

I have tested stuff by hand (I took the code and created some structures -
a bit of workaround let's say). Should I test it with OSSTest too?
What I understood from Wei is that he tested my patch. Maybe I got it wrong.

Thank you all,
Alex

On Fri, Oct 24, 2014 at 11:01 AM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:

> On Thu, 2014-10-23 at 19:08 -0400, Konrad Rzeszutek Wilk wrote:
> > > He said the code *path*, not the code, IOW I would read that as "would
> > > be tested", NOT "has been tested".
> > >
> >
> > Aye. In which case let's wait for Alexandra's response to the testing
> > that Wei asked for.
>
> Alexandra replied to me (privately) a couple of days ago confirming that
> she has tested both code paths.
>
> > On a related note - one can setup OSSTest on ones own network, fairly
> easy right?
>
> Right in "standalone mode". It's far easier than it once was and we've
> been squashing speedbumps as new people try it.
>
> You do need some infrastructure such as PXE and dhcp and at least two
> machines (controller + test host). I think it would be overkill for a
> change of this magnitude (if that's what you were driving at).
>
> I think
>
> http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;h=9a85549cb600fa5be91bcbae6605f78616db63da;hb=2aa4fe7a4b138e5e88387af66f4fe14039a1f7ea
> is the best starting point for osstest, including standalone mode operation.
>
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 2216 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Fri, 2014-10-24 at 11:58 +0300, Alexandra Sandulescu wrote:

Please don't top post on the xen-devel list.

> I have tested stuff by hand (I took the code and created some
> structures - a bit of workaround let's say).

What do you mean here? Have you run the xl command on a real Xen system
with these patch applied?

>  Should I test it with OSSTest too?

If you've tested manually on a real Xen system I don't think this is
necessary, although if you are interested in getting osstest going then
I am quite happy to offer guidance where necessary.

> What I understood from Wei is that he tested my patch. Maybe I got it
> wrong.

That's not how I understood his comment, but I'll let him clarify.

Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu]

[-- Attachment #1.1: Type: text/plain, Size: 279 bytes --]

No I did not. I was easier to test the functions. I used the expected
arguments from .cfg and for networkattach command.
If you want to test using the xl command, I will.
It looked a bit too much for me because  my patch does not really introduce
new code.

Thank you,
Alexandra

[-- Attachment #1.2: Type: text/html, Size: 354 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Fri, 2014-10-24 at 12:12 +0300, Alexandra Sandulescu wrote:
> No I did not. I was easier to test the functions. I used the expected
> arguments from .cfg and for networkattach command.
> 
> If you want to test using the xl command, I will.
> 
> It looked a bit too much for me because  my patch does not really
> introduce new code.

Part of the purpose of this stage of the application process is not just
to change the code but also to start to gain familiarity with the Xen
system and set on up etc.

Although testing the functions in isolation can sometimes be OK I think
given the above it would preferred to setup an actual Xen system and run
the code in a real environment.

Ian.

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu]

[-- Attachment #1.1: Type: text/plain, Size: 60 bytes --]

I have the setup :d

I will test it there too, then.
Thanks

[-- Attachment #1.2: Type: text/html, Size: 117 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Wei Liu]

On Thu, Oct 23, 2014 at 04:08:09PM -0400, Konrad Rzeszutek Wilk wrote:
> On October 23, 2014 3:56:14 AM EDT, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >On Wed, 2014-10-22 at 16:21 -0400, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Oct 22, 2014 at 12:35:58PM +0100, Ian Campbell wrote:
> >> > On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> >> > > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> >> > > adding parse_nic_config function. This function parses
> >configuration
> >> > > data and adds the information into libxl_device_nic struct. It is
> >> > > called in both main_networkattach and parse_config_data functions
> >> > > to replace duplicate code.
> >> > > 
> >> > > Signed-off-by: Alexandra Sandulescu
> ><alecsandra.sandulescu@gmail.com>
> >> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >> > 
> >> > This looks good to me, thanks. In reply to the first posting I
> >asked:
> >> > Did you test both code paths? (wrt cfg file vs xl network-attach
> >usage).
> >> > Did you?
> >> > 
> >> > Konrad, any reply to Wei's pros/cons on this patch for 4.5?
> >> > (<20141021152420.GI10234@zion.uk.xensource.com>)
> >> 
> >> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> 
> >> I am making this based on the fact that:
> >>  - It has run through the OSSTest which does a ton of tests so the
> >>    chance of regression is almost nill.
> >
> >You mean "will", not "has", right? Since it won't be run through
> >osstest
> >until it is committed.
> 
> Has. Wei said it had run through it.
> 

Nope.

Did I get the grammar wrong? Sorry. ;-)

> >
> >Ian.
> 

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Konrad Rzeszutek Wilk]

On Fri, Oct 24, 2014 at 11:58:28AM +0300, Alexandra Sandulescu wrote:
> Hello,
> 
> I have tested stuff by hand (I took the code and created some structures -
> a bit of workaround let's say). Should I test it with OSSTest too?

No need to run the OSSTest, that was more for my own curiosity.

> What I understood from Wei is that he tested my patch. Maybe I got it wrong.
> 
> Thank you all,
> Alex
> 
> On Fri, Oct 24, 2014 at 11:01 AM, Ian Campbell <Ian.Campbell@citrix.com>
> wrote:
> 
> > On Thu, 2014-10-23 at 19:08 -0400, Konrad Rzeszutek Wilk wrote:
> > > > He said the code *path*, not the code, IOW I would read that as "would
> > > > be tested", NOT "has been tested".
> > > >
> > >
> > > Aye. In which case let's wait for Alexandra's response to the testing
> > > that Wei asked for.
> >
> > Alexandra replied to me (privately) a couple of days ago confirming that
> > she has tested both code paths.
> >
> > > On a related note - one can setup OSSTest on ones own network, fairly
> > easy right?
> >
> > Right in "standalone mode". It's far easier than it once was and we've
> > been squashing speedbumps as new people try it.
> >
> > You do need some infrastructure such as PXE and dhcp and at least two
> > machines (controller + test host). I think it would be overkill for a
> > change of this magnitude (if that's what you were driving at).
> >
> > I think
> >
> > http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;h=9a85549cb600fa5be91bcbae6605f78616db63da;hb=2aa4fe7a4b138e5e88387af66f4fe14039a1f7ea
> > is the best starting point for osstest, including standalone mode operation.
> >
> > Ian.
> >
> >

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Konrad Rzeszutek Wilk]

On Fri, Oct 24, 2014 at 09:01:03AM +0100, Ian Campbell wrote:
> On Thu, 2014-10-23 at 19:08 -0400, Konrad Rzeszutek Wilk wrote:
> > > He said the code *path*, not the code, IOW I would read that as "would
> > > be tested", NOT "has been tested".
> > >
> > 
> > Aye. In which case let's wait for Alexandra's response to the testing
> > that Wei asked for.
> 
> Alexandra replied to me (privately) a couple of days ago confirming that
> she has tested both code paths.
> 
> > On a related note - one can setup OSSTest on ones own network, fairly easy right?
> 
> Right in "standalone mode". It's far easier than it once was and we've
> been squashing speedbumps as new people try it.
> 
> You do need some infrastructure such as PXE and dhcp and at least two
> machines (controller + test host). I think it would be overkill for a
> change of this magnitude (if that's what you were driving at).

Heck no. I just need to setup locally to supplement the automated testing
I do now.

> 
> I think
> http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;h=9a85549cb600fa5be91bcbae6605f78616db63da;hb=2aa4fe7a4b138e5e88387af66f4fe14039a1f7ea is the best starting point for osstest, including standalone mode operation.
> 

Thank you.
> Ian.
> 

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu]

[-- Attachment #1.1: Type: text/plain, Size: 56 bytes --]

Tested in Xen setup too. Sorry for the delay

Alexandra

[-- Attachment #1.2: Type: text/html, Size: 98 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [OPW PATCH V4] tools: xl: refactor code to parse network device options

[Ian Campbell]

On Wed, 2014-10-22 at 12:35 +0100, Ian Campbell wrote:
> On Wed, 2014-10-22 at 00:36 +0300, Alexandra Sandulescu wrote:
> > This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> > adding parse_nic_config function. This function parses configuration
> > data and adds the information into libxl_device_nic struct. It is
> > called in both main_networkattach and parse_config_data functions
> > to replace duplicate code.
> > 
> > Signed-off-by: Alexandra Sandulescu <alecsandra.sandulescu@gmail.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> This looks good to me, thanks.

I've now applied this since the freeze is over.

Ian.