Re: [Xen-devel] [OPW PATCH V2] tools: xl: refactor code to parse network device options

[Alexandra Sandulescu <alecsandra.sandulescu@xxxxxxxxx>]

Hello Ian and Wei,

I have taken into consideration your advice and changed the patch.

There are a few things I have to add.

First of all, I did not check the return value of parse_nic_config in parse_config_data function because the initial parse method did not check data validity and filled as much as possible fields in the libxl_device_nic struct.

I have tested the functionality and it worked as expected after the modifications.

I am waiting for a further review.

Thank you very much for your feedback,

Alexandra

On Tue, Oct 21, 2014 at 12:06 AM, Alexandra Sandulescu <alecsandra.sandulescu@xxxxxxxxx> 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@xxxxxxxxx>
---
Âtools/libxl/xl_cmdimpl.c | 203 ++++++++++++++++++-----------------------------
Â1 file changed, 78 insertions(+), 125 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 988ee28..e45067a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -823,6 +823,80 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
  Â}
Â}

+/* 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))
+
+static void replace_string(char **str, const char *val)
+{
+Â Â free(*str);
+Â Â *str = strdup(val);
+}
+
+/* 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,
@@ -1372,7 +1446,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;
@@ -1386,64 +1460,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);
@@ -1957,24 +1974,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))
-
-static void replace_string(char **str, const char *val)
-{
-Â Â free(*str);
-Â Â *str = strdup(val);
-}
-
-
Â/* 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)
@@ -5987,10 +5986,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 +6008,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);
-Â Â Â Â Â Â return 1;
-Â Â Â Â }
+Â Â Â Â Â Â Â if (parse_nic_config(&nic, &config, *argv))
+Â Â Â Â Â Â return 1;
  Â}

  Âif (dryrun_only) {
--
1.9.1

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