[PATCH 0/4] xl: consolidate adhoc parsers

[PATCH 0/4] xl: consolidate adhoc parsers

[Wei Liu]

This patch series consolidates adhoc parsers in xl.

There are currently 4 types of devices:

1. block
2. netowrk
3. vtpm
4. pci

that support hotplug as well as being specified in config file.

Block and pci devices are fine because they use libxlu to parse
configuration strings.

Network and vtpms config parsers are not in a very good state, which
need to be consolidated.

Note that there is code repetition in the newly introduced parser
code. We either need to have very long macro definition or code
repetition. I chose the latter. Let me know your opinion.

Wei.

Wei Liu (4):
  xl: remove unused macros
  xl: wrap long lines where possible
  xl: rework vif config parsing code
  xl: rework vtpm config parsing code

 tools/libxl/xl_cmdimpl.c | 312 +++++++++++++++++++++++++++++++----------------
 1 file changed, 207 insertions(+), 105 deletions(-)

-- 
2.1.4

[PATCH 1/4] xl: remove unused macros

[Wei Liu]

They were added back in 2011 to be used in an adhoc parser which has now
been removed.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..f380799 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -556,14 +556,6 @@ static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a
     return 0;
 }
 
-#define DSTATE_INITIAL   0
-#define DSTATE_TAP       1
-#define DSTATE_PHYSPATH  2
-#define DSTATE_VIRTPATH  3
-#define DSTATE_VIRTTYPE  4
-#define DSTATE_RW        5
-#define DSTATE_TERMINAL  6
-
 static void parse_disk_config_multistring(XLU_Config **config,
                                           int nspecs, const char *const *specs,
                                           libxl_device_disk *disk)
-- 
2.1.4

Re: [PATCH 1/4] xl: remove unused macros

[Ian Campbell]

On Fri, 2016-01-22 at 11:50 +0000, Wei Liu wrote:
> They were added back in 2011 to be used in an adhoc parser which has now
> been removed.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 2/4] xl: wrap long lines where possible

[Wei Liu]

No functional changes introduced.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 107 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 34 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f380799..ff561c3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -237,12 +237,14 @@ static int acquire_lock(void)
     fl.l_len = 0;
     fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
     if (fd_lock < 0) {
-        fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile, errno);
+        fprintf(stderr, "cannot open the lockfile %s errno=%d\n",
+                lockfile, errno);
         return ERROR_FAIL;
     }
     if (fcntl(fd_lock, F_SETFD, FD_CLOEXEC) < 0) {
         close(fd_lock);
-        fprintf(stderr, "cannot set cloexec to lockfile %s errno=%d\n", lockfile, errno);
+        fprintf(stderr, "cannot set cloexec to lockfile %s errno=%d\n",
+                lockfile, errno);
         return ERROR_FAIL;
     }
 get_lock:
@@ -538,12 +540,16 @@ out:
     return ret;
 }
 
-static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a)
+static int parse_action_on_shutdown(const char *buf,
+                                    libxl_action_on_shutdown *a)
 {
-    int i;
+    int i, size;
     const char *n;
 
-    for (i = 0; i < sizeof(action_on_shutdown_names) / sizeof(action_on_shutdown_names[0]); i++) {
+    size = sizeof(action_on_shutdown_names) /
+        sizeof(action_on_shutdown_names[0]);
+
+    for (i = 0; i < size; i++) {
         n = action_on_shutdown_names[i];
 
         if (!n) continue;
@@ -994,7 +1000,8 @@ static int match_option_size(const char *prefix, size_t len,
 /* 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)
+static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config,
+                            char *token)
 {
     char *endptr, *oparg;
     int i;
@@ -1040,7 +1047,8 @@ static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *to
     } 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");
+        fprintf(stderr,
+                "the accel parameter for vifs is currently not supported\n");
     } else {
         fprintf(stderr, "unrecognized argument `%s'\n", token);
         return 1;
@@ -1580,13 +1588,15 @@ static void parse_config_data(const char *config_source,
 
             if (l < LIBXL_TIMER_MODE_DELAY_FOR_MISSED_TICKS ||
                 l > LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING) {
-                fprintf(stderr, "ERROR: invalid value %ld for \"timer_mode\"\n", l);
+                fprintf(stderr,
+                        "ERROR: invalid value %ld for \"timer_mode\"\n", l);
                 exit (1);
             }
             b_info->u.hvm.timer_mode = l;
         } else if (!xlu_cfg_get_string(config, "timer_mode", &buf, 0)) {
             if (libxl_timer_mode_from_string(buf, &b_info->u.hvm.timer_mode)) {
-                fprintf(stderr, "ERROR: invalid value \"%s\" for \"timer_mode\"\n",
+                fprintf(stderr,
+                        "ERROR: invalid value \"%s\" for \"timer_mode\"\n",
                         buf);
                 exit (1);
             }
@@ -1605,7 +1615,8 @@ static void parse_config_data(const char *config_source,
             if (!strcmp(buf, "generate")) {
                 e = libxl_ms_vm_genid_generate(ctx, &b_info->u.hvm.ms_vm_genid);
                 if (e) {
-                    fprintf(stderr, "ERROR: failed to generate a VM Generation ID\n");
+                    fprintf(stderr,
+                            "ERROR: failed to generate a VM Generation ID\n");
                     exit(1);
                 }
             } else if (!strcmp(buf, "none")) {
@@ -1621,7 +1632,8 @@ static void parse_config_data(const char *config_source,
         break;
     case LIBXL_DOMAIN_TYPE_PV:
     {
-        xlu_cfg_replace_string (config, "bootloader", &b_info->u.pv.bootloader, 0);
+        xlu_cfg_replace_string (config, "bootloader",
+                                &b_info->u.pv.bootloader, 0);
         switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args",
                                       &b_info->u.pv.bootloader_args, 1))
         {
@@ -1772,7 +1784,8 @@ static void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
         d_config->num_disks = 0;
         d_config->disks = NULL;
-        while ((buf = xlu_cfg_get_listitem (vbds, d_config->num_disks)) != NULL) {
+        while ((buf = xlu_cfg_get_listitem (vbds, d_config->num_disks))
+               != NULL) {
             libxl_device_disk *disk;
             char *buf2 = strdup(buf);
 
@@ -1788,7 +1801,8 @@ static void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
         d_config->num_vtpms = 0;
         d_config->vtpms = NULL;
-        while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms)) != NULL) {
+        while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms))
+               != NULL) {
             libxl_device_vtpm *vtpm;
             char * buf2 = strdup(buf);
             char *p, *p2;
@@ -1889,7 +1903,8 @@ static void parse_config_data(const char *config_source,
                 exit(1);
             case LIBXL_CHANNEL_CONNECTION_SOCKET:
                 if (!path) {
-                    fprintf(stderr, "channel connection 'socket' requires path=..\n");
+                    fprintf(stderr,
+                            "channel connection 'socket' requires path=..\n");
                     exit(1);
                 }
                 chn->u.socket.path = xstrdup(path);
@@ -1910,7 +1925,8 @@ static void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
-        while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics)) != NULL) {
+        while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics))
+               != NULL) {
             libxl_device_nic *nic;
             char *buf2 = strdup(buf);
             char *p;
@@ -1943,7 +1959,8 @@ skip_nic:
     d_config->vkbs = NULL;
 
     if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
-        while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) {
+        while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs))
+               != NULL) {
             libxl_device_vfb *vfb;
             libxl_device_vkb *vkb;
 
@@ -2462,7 +2479,8 @@ static domain_restart_type handle_domain_death(uint32_t *r_domid,
         event->u.domain_shutdown.shutdown_reason,
         action_on_shutdown_names[action]);
 
-    if (action == LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY || action == LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART) {
+    if (action == LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY ||
+        action == LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART) {
         char *corefile;
         int rc;
 
@@ -2699,7 +2717,8 @@ static uint32_t create_domain(struct domain_create *dom_info)
             restore_source = restore_file;
             restore_fd = open(restore_file, O_RDONLY);
             if (restore_fd == -1) {
-                fprintf(stderr, "Can't open restore file: %s\n", strerror(errno));
+                fprintf(stderr,
+                        "Can't open restore file: %s\n", strerror(errno));
                 return ERROR_INVAL;
             }
             restore_fd_to_close = restore_fd;
@@ -2776,8 +2795,11 @@ static uint32_t create_domain(struct domain_create *dom_info)
         } else {
             ret = libxl_read_file_contents(ctx, config_file,
                                            &config_data, &config_len);
-            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
-                               config_file, strerror(errno)); return ERROR_FAIL; }
+            if (ret) {
+                fprintf(stderr, "Failed to read config file: %s: %s\n",
+                        config_file, strerror(errno));
+                return ERROR_FAIL;
+            }
         }
         if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
@@ -3286,7 +3308,9 @@ int main_memmax(int argc, char **argv)
 
     rc = set_memory_max(domid, mem);
     if (rc) {
-        fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
+        fprintf(stderr,
+                "cannot set domid %d static max memory to : %s\n",
+                domid, mem);
         return 1;
     }
 
@@ -3460,7 +3484,8 @@ static void pcilist(uint32_t domid)
     for (i = 0; i < num; i++) {
         printf("%02x.%01x %04x:%02x:%02x.%01x\n",
                (pcidevs[i].vdevfn >> 3) & 0x1f, pcidevs[i].vdevfn & 0x7,
-               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
+               pcidevs[i].domain,
+               pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
         libxl_device_pci_dispose(&pcidevs[i]);
     }
     free(pcidevs);
@@ -3492,7 +3517,8 @@ static void pcidetach(uint32_t domid, const char *bdf, int force)
     if (!config) { perror("xlu_cfg_inig"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
-        fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr,
+                "pci-detach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
     if (force)
@@ -3534,7 +3560,8 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs)
     if (!config) { perror("xlu_cfg_inig"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
-        fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr,
+                "pci-attach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
     libxl_device_pci_add(ctx, domid, &pcidev, 0);
@@ -3574,7 +3601,8 @@ static void pciassignable_list(void)
         return;
     for (i = 0; i < num; i++) {
         printf("%04x:%02x:%02x.%01x\n",
-               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
+               pcidevs[i].domain,
+               pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
         libxl_device_pci_dispose(&pcidevs[i]);
     }
     free(pcidevs);
@@ -3603,7 +3631,9 @@ static void pciassignable_add(const char *bdf, int rebind)
     if (!config) { perror("xlu_cfg_init"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
-        fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr,
+                "pci-assignable-add: malformed BDF specification \"%s\"\n",
+                bdf);
         exit(2);
     }
     libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
@@ -3638,7 +3668,9 @@ static void pciassignable_remove(const char *bdf, int rebind)
     if (!config) { perror("xlu_cfg_init"); exit(-1); }
 
     if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
-        fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
+        fprintf(stderr,
+                "pci-assignable-remove: malformed BDF specification \"%s\"\n",
+                bdf);
         exit(2);
     }
     libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
@@ -5817,7 +5849,8 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
     return 0;
 }
 
-static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
+static int sched_credit_params_set(int poolid,
+                                   libxl_sched_credit_params *scinfo)
 {
     if (libxl_sched_credit_params_set(ctx, poolid, scinfo)) {
         fprintf(stderr, "libxl_sched_credit_params_set failed.\n");
@@ -5827,7 +5860,8 @@ static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo
     return 0;
 }
 
-static int sched_credit_params_get(int poolid, libxl_sched_credit_params *scinfo)
+static int sched_credit_params_get(int poolid,
+                                   libxl_sched_credit_params *scinfo)
 {
     if (libxl_sched_credit_params_get(ctx, poolid, scinfo)) {
         fprintf(stderr, "libxl_sched_credit_params_get failed.\n");
@@ -6699,8 +6733,9 @@ int main_blocklist(int argc, char **argv)
             if (!libxl_device_disk_getinfo(ctx, domid, &disks[i], &diskinfo)) {
                 /*      Vdev BE   hdl  st   evch rref BE-path*/
                 printf("%-5d %-3d %-6d %-5d %-6d %-8d %-30s\n",
-                       diskinfo.devid, diskinfo.backend_id, diskinfo.frontend_id,
-                       diskinfo.state, diskinfo.evtch, diskinfo.rref, diskinfo.backend);
+                       diskinfo.devid, diskinfo.backend_id,
+                       diskinfo.frontend_id, diskinfo.state,
+                       diskinfo.evtch, diskinfo.rref, diskinfo.backend);
                 libxl_diskinfo_dispose(&diskinfo);
             }
             libxl_device_disk_dispose(&disks[i]);
@@ -6880,9 +6915,11 @@ static char *uptime_to_string(unsigned long uptime, int short_mode)
             xasprintf(&time_string, "%2d:%02d", hour, min);
     else
         if (day > 1)
-            xasprintf(&time_string, "%d days, %2d:%02d:%02d", day, hour, min, sec);
+            xasprintf(&time_string, "%d days, %2d:%02d:%02d",
+                      day, hour, min, sec);
         else if (day == 1)
-            xasprintf(&time_string, "%d day, %2d:%02d:%02d", day, hour, min, sec);
+            xasprintf(&time_string, "%d day, %2d:%02d:%02d",
+                      day, hour, min, sec);
         else
             xasprintf(&time_string, "%2d:%02d:%02d", hour, min, sec);
 
@@ -7922,7 +7959,9 @@ int main_setenforce(int argc, char **argv)
             fprintf(stderr, "Flask XSM disabled\n");
         }
         else
-            fprintf(stderr, "error occured while setting enforcing mode (%i)\n", ret);
+            fprintf(stderr,
+                    "error occured while setting enforcing mode (%i)\n",
+                    ret);
     }
 
     return ret;
-- 
2.1.4

Re: [PATCH 2/4] xl: wrap long lines where possible

[Ian Campbell]

On Fri, 2016-01-22 at 11:50 +0000, Wei Liu wrote:
> No functional changes introduced.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(unread)

[PATCH 3/4] xl: rework vif config parsing code

[Wei Liu]

The original parse_nic_config was in fact only parsing tokens.
main_networkattach erroneously used it to parse raw config string, which
led to xl network-attach not respecting network configuration syntax.

Rework vif config parser:

1. Extract the snippet in parse_config_data to parse_nic_config_one.
2. Rename original parse_nic_config to parse_nic_config_token.
3. Provide _multistring variant.
4. Implement parse_nic_config with _multistring variant.
5. Use _multistring variant in main_networkattach.

Finally the call chain becomes: parse_nic_config ->
parse_nic_config_multistring -> parse_nic_config_one ->
parse_nic_config_token.

Some other less important changes:

1. Change prototypes of parse_nic_config functions to match
   block counterpart.
2. parse_nic_config will now exit if parsing failed.
3. main_networkattach doesn't call set_default_nic_values anymore.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 82 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ff561c3..f736f95 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1000,8 +1000,9 @@ static int match_option_size(const char *prefix, size_t len,
 /* 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)
+static int parse_nic_config_token(XLU_Config **config,
+                                  char *token,
+                                  libxl_device_nic *nic)
 {
     char *endptr, *oparg;
     int i;
@@ -1056,6 +1057,59 @@ static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config,
     return 0;
 }
 
+static int parse_nic_config_one(XLU_Config **config,
+                                const char *config_str,
+                                libxl_device_nic *nic)
+{
+    char *buf = xstrdup(config_str);
+    char *p;
+    int ret;
+
+    set_default_nic_values(nic);
+
+    p = strtok(buf, ",");
+    if (!p) {
+        ret = 0;
+        goto out;
+    }
+
+    do {
+        while (*p == ' ')
+            p++;
+        ret = parse_nic_config_token(config, p, nic);
+    } while ((p = strtok(NULL, ",")) != NULL && ret == 0);
+
+out:
+    free(buf);
+
+    return ret;
+}
+
+static void parse_nic_config_multistring(XLU_Config **config,
+                                         int nspecs, const char *const *specs,
+                                         libxl_device_nic *nic)
+{
+    int i;
+
+    libxl_device_nic_init(nic);
+
+    if (!*config) {
+        *config = xlu_cfg_init(stderr, "command line");
+        if (!*config) { perror("xlu_cfg_init"); exit(-1); }
+    }
+
+    for (i = 0; i < nspecs; i++) {
+        if (parse_nic_config_one(config, specs[i], nic))
+            exit(EXIT_FAILURE);
+    }
+}
+
+static void parse_nic_config(XLU_Config **config, const char *buf,
+                             libxl_device_nic *nic)
+{
+    parse_nic_config_multistring(config, 1, &buf, nic);
+}
+
 static unsigned long parse_ulong(const char *str)
 {
     char *endptr;
@@ -1928,24 +1982,10 @@ 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;
-
             nic = ARRAY_EXTEND_INIT(d_config->nics,
                                     d_config->num_nics,
                                     libxl_device_nic_init);
-            set_default_nic_values(nic);
-
-            p = strtok(buf2, ",");
-            if (!p)
-                goto skip_nic;
-            do {
-                while (*p == ' ')
-                    p++;
-                parse_nic_config(nic, &config, p);
-            } while ((p = strtok(NULL, ",")) != NULL);
-skip_nic:
-            free(buf2);
+            parse_nic_config(&config, buf, nic);
         }
     }
 
@@ -6532,12 +6572,10 @@ int main_networkattach(int argc, char **argv)
     }
 
     libxl_device_nic_init(&nic);
-    set_default_nic_values(&nic);
 
-    for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
-        if (parse_nic_config(&nic, &config, *argv))
-            return 1;
-    }
+    optind++;
+    parse_nic_config_multistring(&config, argc-optind,
+                                 (const char* const*) argv + optind, &nic);
 
     if (dryrun_only) {
         char *json = libxl_device_nic_to_json(ctx, &nic);
-- 
2.1.4

[PATCH 4/4] xl: rework vtpm config parsing code

[Wei Liu]

Follow the same pattern as vif config parse to introduce
parse_vtpm_config_token, parse_vtpm_config_one,
parse_vtpm_config_multistring and parse_vtpm_config. Then replace
open-coded parsing code with appropriate functions.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 119 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 43 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f736f95..1444b0b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1110,6 +1110,76 @@ static void parse_nic_config(XLU_Config **config, const char *buf,
     parse_nic_config_multistring(config, 1, &buf, nic);
 }
 
+static int parse_vtpm_config_token(XLU_Config **config, char *token,
+                                   libxl_device_vtpm *vtpm)
+{
+    char *oparg;
+
+    if (MATCH_OPTION("uuid", token, oparg)) {
+        if(libxl_uuid_from_string(&vtpm->uuid, oparg)) {
+            fprintf(stderr, "Invalid uuid specified (%s)\n", oparg);
+            return 1;
+        }
+    } else if (MATCH_OPTION("backend", token, oparg)) {
+        replace_string(&vtpm->backend_domname, oparg);
+    } else {
+        fprintf(stderr, "unrecognized argument `%s'\n", token);
+        return 1;
+    }
+
+    return 0;
+}
+
+static int parse_vtpm_config_one(XLU_Config **config, const char *config_str,
+                                 libxl_device_vtpm *vtpm)
+{
+    char *buf = xstrdup(config_str);
+    char *p;
+    int ret;
+
+    p = strtok(buf, ",");
+    if (!p) {
+        ret = 0;
+        goto out;
+    }
+
+    do {
+        while (*p == ' ')
+            p++;
+        ret = parse_vtpm_config_token(config, p, vtpm);
+    } while ((p = strtok(NULL, ",")) != NULL && ret == 0);
+
+out:
+    free(buf);
+
+    return ret;
+}
+
+static void parse_vtpm_config_multistring(XLU_Config **config,
+                                          int nspecs, const char *const *specs,
+                                          libxl_device_vtpm *vtpm)
+{
+    int i;
+
+    libxl_device_vtpm_init(vtpm);
+
+    if (!*config) {
+        *config = xlu_cfg_init(stderr, "command line");
+        if (!*config) { perror("xlu_cfg_init"); exit(-1); }
+    }
+
+    for (i = 0; i < nspecs; i++) {
+        if (parse_vtpm_config_one(config, specs[i], vtpm))
+            exit(EXIT_FAILURE);
+    }
+}
+
+static void parse_vtpm_config(XLU_Config **config, const char *buf,
+                              libxl_device_vtpm *vtpm)
+{
+    parse_vtpm_config_multistring(config, 1, &buf, vtpm);
+}
+
 static unsigned long parse_ulong(const char *str)
 {
     char *endptr;
@@ -1858,42 +1928,17 @@ static void parse_config_data(const char *config_source,
         while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms))
                != NULL) {
             libxl_device_vtpm *vtpm;
-            char * buf2 = strdup(buf);
-            char *p, *p2;
-            bool got_backend = false;
 
             vtpm = ARRAY_EXTEND_INIT(d_config->vtpms,
                                      d_config->num_vtpms,
                                      libxl_device_vtpm_init);
 
-            p = strtok(buf2, ",");
-            if(p) {
-               do {
-                  while(*p == ' ')
-                     ++p;
-                  if ((p2 = strchr(p, '=')) == NULL)
-                     break;
-                  *p2 = '\0';
-                  if (!strcmp(p, "backend")) {
-                     vtpm->backend_domname = strdup(p2 + 1);
-                     got_backend = true;
-                  } else if(!strcmp(p, "uuid")) {
-                     if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
-                        fprintf(stderr,
-                              "Failed to parse vtpm UUID: %s\n", p2 + 1);
-                        exit(1);
-                    }
-                  } else {
-                     fprintf(stderr, "Unknown string `%s' in vtpm spec\n", p);
-                     exit(1);
-                  }
-               } while ((p = strtok(NULL, ",")) != NULL);
-            }
-            if(!got_backend) {
+            parse_vtpm_config(&config, buf, vtpm);
+
+            if(!vtpm->backend_domname) {
                fprintf(stderr, "vtpm spec missing required backend field!\n");
                exit(1);
             }
-            free(buf2);
         }
     }
 
@@ -6812,8 +6857,8 @@ int main_vtpmattach(int argc, char **argv)
 {
     int opt;
     libxl_device_vtpm vtpm;
-    char *oparg;
     uint32_t domid;
+    XLU_Config *config = 0;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "vtpm-attach", 1) {
         /* No options */
@@ -6825,20 +6870,8 @@ int main_vtpmattach(int argc, char **argv)
     }
     ++optind;
 
-    libxl_device_vtpm_init(&vtpm);
-    for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) {
-        if (MATCH_OPTION("uuid", *argv, oparg)) {
-            if(libxl_uuid_from_string(&(vtpm.uuid), oparg)) {
-                fprintf(stderr, "Invalid uuid specified (%s)\n", oparg);
-                return 1;
-            }
-        } else if (MATCH_OPTION("backend", *argv, oparg)) {
-            replace_string(&vtpm.backend_domname, oparg);
-        } else {
-            fprintf(stderr, "unrecognized argument `%s'\n", *argv);
-            return 1;
-        }
-    }
+    parse_vtpm_config_multistring(&config, argc-optind,
+                                  (const char* const*) argv + optind, &vtpm);
 
     if(dryrun_only) {
        char* json = libxl_device_vtpm_to_json(ctx, &vtpm);
-- 
2.1.4

Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Ian Campbell]

On Fri, 2016-01-22 at 11:50 +0000, Wei Liu wrote:
> This patch series consolidates adhoc parsers in xl.
> 
> There are currently 4 types of devices:
> 
> 1. block
> 2. netowrk
> 3. vtpm
> 4. pci
> 
> that support hotplug as well as being specified in config file.
> 
> Block and pci devices are fine because they use libxlu to parse
> configuration strings.
> 
> Network and vtpms config parsers are not in a very good state, which
> need to be consolidated.
> 
> Note that there is code repetition in the newly introduced parser
> code. We either need to have very long macro definition or code
> repetition. I chose the latter. Let me know your opinion.

You could make it take function pointers by using void * to point to the
objects. There's a (small) risk of mismatching the point and the callback,
but not significant enough to worry about IMHO.

That said, when I mentioned helpers originally I was imagining they would
parse their input into a data structure containing (in some form or other)
a list of key value pairs (taking into account the behaviour desired on
duplication of keys etc) which the callers would use opaquely up front and
then iterate over the results in some way (either a foreach helper macro,
or first() and next() type helpers etc).

Ian.

Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Jim Fehlig]

Wei Liu wrote:
> This patch series consolidates adhoc parsers in xl.

Hi Wei,

I never tested or reviewed this series after seeing Ian's comments. Did you have
time to work on a V2? Or did I miss a V2? :-) Let me know if I can help.

Regards,
Jim

Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Wei Liu]

On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote:
> Wei Liu wrote:
> > This patch series consolidates adhoc parsers in xl.
> 
> Hi Wei,
> 
> I never tested or reviewed this series after seeing Ian's comments. Did you have
> time to work on a V2? Or did I miss a V2? :-) Let me know if I can help.
>

Sorry, this series fell through the crack -- it was only the work of
one afternoon when I had some free cycles. Now I'm busy with other
stuff and haven't had the time to pick it up again.

One thing I'm not entirely happy with writing this in plain C is that
there is subtle semantics difference from the ones generated by
flex/bison even if they expose similar APIs. For example, the
functions to parse disk spec won't allow you to set same attribute
twice, while the work present in this function allows you to do
that. It's cumbersome to implement the same functionality with open
coding in C IMHO.

To get to the point where I feel happy requires quite a bit of work,
so I put "consolidating xl ad hoc parser" (a super set of this series)
to this year's GSoC this morning. Basically I want to consolidate ad
hoc parser, let them have similar semantics if possible, and then
provide a test suite so that it won't be broken by mistake.

That being said, perfection is the enemy of good. If you think this is
important, I will try to find some time to refresh this series; but I
don't have time for the test suite yet. You're welcome to take over
this series if you have time.

Wei.

(Please forgive my errors in grammar etc, it's a bit late at night here...)

> Regards,
> Jim

Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Ian Campbell]

On Tue, 2016-02-16 at 23:20 +0000, Wei Liu wrote:
> On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote:
> > Wei Liu wrote:
> > > This patch series consolidates adhoc parsers in xl.
> > 
> > Hi Wei,
> > 
> > I never tested or reviewed this series after seeing Ian's comments. Did
> > you have
> > time to work on a V2? Or did I miss a V2? :-) Let me know if I can
> > help.
> > 
> 
> Sorry, this series fell through the crack -- it was only the work of
> one afternoon when I had some free cycles. Now I'm busy with other
> stuff and haven't had the time to pick it up again.
> 
> One thing I'm not entirely happy with writing this in plain C is that
> there is subtle semantics difference from the ones generated by
> flex/bison even if they expose similar APIs. For example, the
> functions to parse disk spec won't allow you to set same attribute
> twice, while the work present in this function allows you to do
> that. It's cumbersome to implement the same functionality with open
> coding in C IMHO.

Take care -- the xlu disk parser has a lot of additional complexity and
scaffolding to support the "legacy" xm style device specifications with
positional parameters and strange (ignored by xl) prefixes, trying to merge
that with other device types is likely to result in unwanted/unmanageable
complexity IMHO.

I'm also not sure that switching to flex+bison for the rest is going to
solve the problems you mention above (disallowing setting something twice
etc). flex+bison will take care nicely of things like tokenising and
parsing the input (i.e. it is a nice replacement for strtok and friends),
but ultimately the keys and values need to be exposed to the user somehow
i.e. put into some data structure.

That data structure could either be completely abstract (i.e. just provide
"lookup by key" functionality, via some sort of hash or list etc, which is
basically what the xlu cfg stuff does) or device type specific (i.e. fills
in a given struct based on a knowledge of the value keys for that struct,
which is what the disk stuff does) and it's in that code where you would
need to handle things like what to do with repeated keys.

That is essentially just done by the C snippets in the bison rules or the
scaffolding surrounding the bison, it's not something that bison inherently
deals with.

So ultimately you need to deal with the question of what the semantics of
setting a given key a second time are (error, take the first value, take
the last value, per-key settings etc) in C code, even if you are using
bison. That's slightly easier if you have a parser per device type, since
you can just check the specific field in the specific struct for being
unset or not before making a choice, but even in abstract code it's just a
case of checking in the list/table/hash you are constructing during parsing
for the key and reacting accordingly, but in both cases you are doing so in
C not in "bison".

Ian.

Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Wei Liu]

On Wed, Feb 17, 2016 at 10:01:37AM +0000, Ian Campbell wrote:
> On Tue, 2016-02-16 at 23:20 +0000, Wei Liu wrote:
> > On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote:
> > > Wei Liu wrote:
> > > > This patch series consolidates adhoc parsers in xl.
> > > 
> > > Hi Wei,
> > > 
> > > I never tested or reviewed this series after seeing Ian's comments. Did
> > > you have
> > > time to work on a V2? Or did I miss a V2? :-) Let me know if I can
> > > help.
> > > 
> > 
> > Sorry, this series fell through the crack -- it was only the work of
> > one afternoon when I had some free cycles. Now I'm busy with other
> > stuff and haven't had the time to pick it up again.
> > 
> > One thing I'm not entirely happy with writing this in plain C is that
> > there is subtle semantics difference from the ones generated by
> > flex/bison even if they expose similar APIs. For example, the
> > functions to parse disk spec won't allow you to set same attribute
> > twice, while the work present in this function allows you to do
> > that. It's cumbersome to implement the same functionality with open
> > coding in C IMHO.
> 
> Take care -- the xlu disk parser has a lot of additional complexity and
> scaffolding to support the "legacy" xm style device specifications with
> positional parameters and strange (ignored by xl) prefixes, trying to merge
> that with other device types is likely to result in unwanted/unmanageable
> complexity IMHO.
> 
> I'm also not sure that switching to flex+bison for the rest is going to
> solve the problems you mention above (disallowing setting something twice
> etc). flex+bison will take care nicely of things like tokenising and
> parsing the input (i.e. it is a nice replacement for strtok and friends),

Yes, this is what I'm after as first step -- it's all very vague idea in
my head at the moment. I was thinking we should use flex and bison to
tokenise the input, instead of doing everything by hand.

> but ultimately the keys and values need to be exposed to the user somehow
> i.e. put into some data structure.
> 
> That data structure could either be completely abstract (i.e. just provide
> "lookup by key" functionality, via some sort of hash or list etc, which is
> basically what the xlu cfg stuff does) or device type specific (i.e. fills
> in a given struct based on a knowledge of the value keys for that struct,
> which is what the disk stuff does) and it's in that code where you would
> need to handle things like what to do with repeated keys.
> 
> That is essentially just done by the C snippets in the bison rules or the
> scaffolding surrounding the bison, it's not something that bison inherently
> deals with.
> 
> So ultimately you need to deal with the question of what the semantics of
> setting a given key a second time are (error, take the first value, take
> the last value, per-key settings etc) in C code, even if you are using
> bison. That's slightly easier if you have a parser per device type, since
> you can just check the specific field in the specific struct for being
> unset or not before making a choice, but even in abstract code it's just a
> case of checking in the list/table/hash you are constructing during parsing
> for the key and reacting accordingly, but in both cases you are doing so in
> C not in "bison".
> 

Right, I do understand this.

I'm heavily biased towards formalising everything in BNF -- everyone who
provides a new device type should provide a parser for the configuration
string in BNF, as well as some test cases in our suite. I know this is a
bit unsocial so I'm trying to figure out a way if we can provide some
sensible framework to do that. It's not going to be easy. And whether
the return of investment is worthy of the effort is unclear. That's why
I propose it to be a GSoC project.

Again, perfection is the enemy of good. I think having everything done
in open coding C is fine. But since we need to have similar semantics
for all parsers we need to disallow setting same value twice, the
boilerplate for that is just tedious to write.

Wei.

> Ian.