[PATCH] tools: Refactor code in libxl/xl_cmdimpl.c

[PATCH] tools: Refactor code in libxl/xl_cmdimpl.c

[Alexandra Sandulescu]

This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
adding set_device_data 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.
---
 tools/libxl/xl_cmdimpl.c | 160 +++++++++++++++++------------------------------
 1 file changed, 57 insertions(+), 103 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 988ee28..449aa91 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -823,6 +823,8 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
     }
 }
 
+static int set_device_data(libxl_device_nic *nic, XLU_Config **config, char *token);
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -1389,61 +1391,7 @@ static void parse_config_data(const char *config_source,
                 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);
-                }
+                set_device_data(nic, &config, p);
             } while ((p = strtok(NULL, ",")) != NULL);
 skip_nic:
             free(buf2);
@@ -1974,6 +1922,59 @@ static void replace_string(char **str, const char *val)
     *str = strdup(val);
 }
 
+static int set_device_data(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;
+}
 
 /* Preserve a copy of a domain under a new name. Updates *r_domid */
 static int preserve_domain(uint32_t *r_domid, libxl_event *event,
@@ -5987,10 +5988,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 */
@@ -6013,50 +6010,7 @@ 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);
-            return 1;
-        }
+      if (set_device_data(&nic, &config, *argv)) { return 1;}
     }
 
     if (dryrun_only) {
-- 
1.9.1

Re: [PATCH] tools: Refactor code in libxl/xl_cmdimpl.c

[Wei Liu]

On Mon, Oct 20, 2014 at 03:01:28AM +0300, Alexandra Sandulescu wrote:
> This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> adding set_device_data 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.
> ---
>  tools/libxl/xl_cmdimpl.c | 160 +++++++++++++++++------------------------------
>  1 file changed, 57 insertions(+), 103 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 988ee28..449aa91 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -823,6 +823,8 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
>      }
>  }
>  
> +static int set_device_data(libxl_device_nic *nic, XLU_Config **config, char *token);
> +

Since this function operates on libxl_device_nic, it should be named
accordingly.

>  static void parse_config_data(const char *config_source,
>                                const char *config_data,
>                                int config_len,
> @@ -1389,61 +1391,7 @@ static void parse_config_data(const char *config_source,
>                  if ((p2 = strchr(p, '=')) == NULL)
>                      break;
>                  *p2 = '\0';

You can get rid of p2, can't you?

[...]
> -            fprintf(stderr, "unrecognized argument `%s'\n", *argv);
> -            return 1;
> -        }
> +      if (set_device_data(&nic, &config, *argv)) { return 1;}

Coding style.

>      }
>  
>      if (dryrun_only) {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] tools: Refactor code in libxl/xl_cmdimpl.c

[Ian Campbell]

On Mon, 2014-10-20 at 03:01 +0300, Alexandra Sandulescu wrote:
> This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> adding set_device_data 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.

Thanks. There's a couple of procedural things which need sorting and
some pretty minor coding style nits but on the whole this is a good
first submission.

The first thing this needs is a signed-off-by line as described at
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch.
This is required to ensure that you have agreed to license the code
correctly for the Xen project.

Secondly both the subject line and the new function name should use the
word "network" or "net"/"nic" etc in them to make it clear what they
relate to. e.g. for the subject "tools: xl: refactor code to parse
network device options".

> ---
>  tools/libxl/xl_cmdimpl.c | 160 +++++++++++++++++------------------------------
>  1 file changed, 57 insertions(+), 103 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 988ee28..449aa91 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -823,6 +823,8 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
>      }
>  }
>  
> +static int set_device_data(libxl_device_nic *nic, XLU_Config **config, char *token);

parse_nic_config() perhaps? Or since this appears to deal with a single
foo=bar value perhaps parse_nic_keyvalue()? A doc comment to confirm the
expected behaviour would be good, since the two callers do have subtly
different semantics and expectations. Did you test both code paths?

Generally we prefer to order things such that these forward declarations
of static functions are not needed. IOW you could move the
implementation here instead.

 +[...]
> +                set_device_data(nic, &config, p);

Check the return value?

> +      if (set_device_data(&nic, &config, *argv)) { return 1;}

The xl  coding style (see, tools/libxl/CODING_STYLE) is 4 spaces, not 2
as you have here. The "return"  would normally be on  the next line and
the {}'s aren't needed e.g.:
            if (set_device_data(&nic, &config, *argv))
                return 1;

Although it is suggested in coding style that
            if (set_device_data(&nic, &config, *argv)) return 1;
might be allowable I think this is best avoided when the condition is
not a trivial comparison of an rc variable.

Ian.