[PATCH] libxl: move TOSTRING to libxl_internal.h

[PATCH] libxl: move TOSTRING to libxl_internal.h

[Ian Jackson]

From: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 tools/libxl/libxl.c          |    2 --
 tools/libxl/libxl_internal.h |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e4704d1..af40d02 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -38,8 +38,6 @@
 
 #define PAGE_TO_MEMKB(pages) ((pages) * 4)
 #define BACKEND_STRING_SIZE 5
-#define STRINGIFY(x) #x
-#define TOSTRING(x) STRINGIFY(x)
 
 int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f698d36..8a2e967 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -330,4 +330,7 @@ _hidden int libxl__error_set(libxl__gc *gc, int code);
 _hidden int libxl__file_reference_map(libxl_file_reference *f);
 _hidden int libxl__file_reference_unmap(libxl_file_reference *f);
 
+#define STRINGIFY(x) #x
+#define TOSTRING(x) STRINGIFY(x)
+
 #endif
-- 
1.5.6.5

[PATCH] libxl: new xlu_disk_parse function

[Ian Jackson]

From: Ian Jackson <ijackson@chiark.greenend.org.uk>

Introduce new flex/regexp-based parser for disk configuration strings.

Callers will be updated in following patches.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 config/StdGNU.mk              |    1 +
 tools/libxl/Makefile          |    9 +-
 tools/libxl/libxlu_cfg.c      |   42 +++++++---
 tools/libxl/libxlu_disk.c     |  164 +++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxlu_disk_i.h   |   19 +++++
 tools/libxl/libxlu_disk_l.l   |   54 ++++++++++++++
 tools/libxl/libxlu_internal.h |   20 +++++
 tools/libxl/libxlutil.h       |   12 +++
 8 files changed, 304 insertions(+), 17 deletions(-)
 create mode 100644 tools/libxl/libxlu_disk.c
 create mode 100644 tools/libxl/libxlu_disk_i.h
 create mode 100644 tools/libxl/libxlu_disk_l.l

diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 25aeb4d..9b331f0 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -67,6 +67,7 @@ CURSES_LIBS = -lncurses
 PTHREAD_LIBS = -lpthread
 UTIL_LIBS = -lutil
 DLOPEN_LIBS = -ldl
+PCRE_LIBS = -lpcre
 
 SONAME_LDFLAG = -soname
 SHLIB_LDFLAGS = -shared
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index a7b1d51..f0c473f 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -22,7 +22,7 @@ endif
 LIBXL_LIBS =
 LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
 
-LIBXLU_LIBS =
+LIBXLU_LIBS = $(PCRE_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
 ifeq ($(LIBXL_BLKTAP),y)
@@ -38,9 +38,10 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_internal.o libxl_utils.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += _libxl_types.o
 
-AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h
-AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
-LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o
+AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h libxlu_disk_l.h
+AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c libxlu_disk_l.c
+LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
+	libxlu_disk_l.o libxlu_disk.o
 
 CLIENTS = xl
 
diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index f947c21..fd3e102 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -15,6 +15,7 @@ XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename) {
     if (!cfg->filename) { free(cfg); return 0; }
 
     cfg->settings= 0;
+    cfg->disk_re= 0;
     return cfg;
 }
 
@@ -38,6 +39,29 @@ static int ctx_prep(CfgParseContext *ctx, XLU_Config *cfg) {
 
 static void ctx_dispose(CfgParseContext *ctx) {
     if (ctx->scanner) xlu__cfg_yylex_destroy(ctx->scanner);
+    if (ctx->disk_re) pcre_free(ctx->disk_re);
+}
+
+int xlu__scanner_string_prep(CfgParseContext *ctx, YY_BUFFER_STATE *buf,
+			     XLU_Config *cfg, const char *data, int length) {
+    int e;
+
+    e= ctx_prep(ctx, cfg);
+    if (e) return e;
+
+    *buf = xlu__cfg_yy_scan_bytes(data, length, ctx->scanner);
+    if (!*buf) {
+        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
+                cfg->filename);
+        return ENOMEM;
+    }
+
+    return 0;
+}
+
+void xlu__scanner_string_dispose(CfgParseContext *ctx, YY_BUFFER_STATE *buf) {
+    if (*buf) xlu__cfg_yy_delete_buffer(*buf, ctx->scanner);
+    ctx_dispose(ctx);
 }
 
 static void parse(CfgParseContext *ctx) {
@@ -87,25 +111,17 @@ int xlu_cfg_readfile(XLU_Config *cfg, const char *real_filename) {
 
 int xlu_cfg_readdata(XLU_Config *cfg, const char *data, int length) {
     int e;
-    YY_BUFFER_STATE buf= 0;
-
+    YY_BUFFER_STATE buf=0;
     CfgParseContext ctx;
-    e= ctx_prep(&ctx, cfg);
-    if (e) { ctx.err= e; goto xe; }
+    ctx.scanner=0;
 
-    buf = xlu__cfg_yy_scan_bytes(data, length, ctx.scanner);
-    if (!buf) {
-        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
-                cfg->filename);
-        ctx.err= ENOMEM;
-        goto xe;
-    }
+    e = xlu__scanner_string_prep(&ctx,&buf, cfg,data,length);
+    if (e) { ctx.err = e; goto xe; }
 
     parse(&ctx);
 
  xe:
-    if (buf) xlu__cfg_yy_delete_buffer(buf, ctx.scanner);
-    ctx_dispose(&ctx);
+    xlu__scanner_string_dispose(&ctx,&buf);
 
     return ctx.err;
 }
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
new file mode 100644
index 0000000..be523f7
--- /dev/null
+++ b/tools/libxl/libxlu_disk.c
@@ -0,0 +1,164 @@
+
+/* Parsing the disk spec a tricky problem, because the target
+ * string might contain "," which is the delimiter we use for
+ * stripping off things on the RHS, and ":", which is the delimiter
+ * we use for stripping off things on the LHS.
+ *
+ * For the LHS we use a flex scanner, see libxlu_disk_l.l.
+ * That chops prefixes like "phy:" from the front of the string,
+ * accumulating information into the disk structure.
+ *
+ * For the RHS, we use a regexp using pcre.
+ */
+
+#include "libxlu_internal.h"
+#include "libxlu_disk_l.h"
+#include "libxlu_disk_i.h"
+#include "libxlu_cfg_i.h"
+
+static void err_core(DiskParseContext *dpc, const char *message,
+		     const char *in, int inlen) {
+    fprintf(dpc->ctx.cfg->report,
+	    "%s: config parsing error in disk specification:"
+	    " %s: near `%s' in `%.*s'\n",
+	    dpc->ctx.cfg->filename, message, dpc->spec, inlen, in);
+    if (!dpc->ctx.err) dpc->ctx.err= EINVAL;
+}
+
+void xlu__disk_err(DiskParseContext *dpc, const char *message) {
+    err_core(dpc, message,
+	     xlu__disk_yyget_text(dpc->ctx.scanner),
+	     xlu__disk_yyget_leng(dpc->ctx.scanner));
+}
+
+void xlu__disk_rhs(DiskParseContext *dpc) {
+
+    /* variables used by pcre */
+#define WORKSPACE_SIZE 20 /* wtf no manifest constant from pcre */
+#define OVECTOR_SIZE 10
+    int ovector[OVECTOR_SIZE];
+    int workspace[WORKSPACE_SIZE];
+
+    int rc, len;
+    const char *text;
+    libxl_device_disk *disk = dpc->disk;
+
+    static const char *re_text=
+	"(.*),(?:/dev/)?([0-9a-zA-Z]*)(:[0-9a-z]+)?(,[0-9a-z]*)?"
+	/* 1:target     2:vdev         3:type        4:attrib (r/w) */;
+
+    /* compile the regexp if we haven't got one already */
+    if (!dpc->ctx.disk_re) {
+	const char *re_errstr;
+	int re_errcode, re_erroffset;
+
+	dpc->ctx.disk_re = pcre_compile2(re_text, PCRE_DOTALL,
+					 &re_errcode, &re_errstr, &re_erroffset,
+					 0);
+	if (!dpc->ctx.disk_re) {
+	    if (re_errcode == 21 /* wtf, no manifest constant */) {
+		dpc->ctx.err = ENOMEM;
+		return;
+	    }
+	    fprintf(dpc->ctx.cfg->report, "config parsing:"
+		    " unable to compile regexp: %s [%d] (at `%s')", 
+		    re_errstr, re_errcode, re_text+re_erroffset);
+	    dpc->ctx.err = EIO;
+	    return;
+	}
+    }
+
+    /* actually run the regexp match */
+
+    text = xlu__disk_yyget_text(dpc->ctx.scanner);
+    len = xlu__disk_yyget_leng(dpc->ctx.scanner);
+
+    rc = pcre_dfa_exec(dpc->ctx.disk_re, 0,
+		       text, len, 0,
+		       PCRE_ANCHORED,
+		       ovector, OVECTOR_SIZE,
+		       workspace, WORKSPACE_SIZE);
+    switch (rc) {
+    case PCRE_ERROR_NOMATCH:
+	xlu__disk_err(dpc, "bad syntax for target, or missing vdev");
+	return;
+    case 2:
+    case 3:
+    case 4:
+	break;
+    default:
+	abort();
+    }
+
+    /* macros for processing info from capturing parens; see pcreapi(3) */
+
+#define CAPTURED(cap) (START((cap)) >= 0)
+#define START(cap) (ovector[(cap)*2])
+#define END(cap)   (ovector[(cap)*2+1])
+#define LEN(cap)   (END((cap)) - START((cap)))
+#define TEXT(cap)  (text + START((cap)))
+
+#define STORE(cap, member) do{					\
+    assert(CAPTURED((cap)));					\
+    free(disk->member);						\
+    disk->member = malloc(LEN((cap)) + 1);			\
+    if (!disk->member) { dpc->ctx.err = ENOMEM; return; }	\
+    memcpy(disk->member, TEXT((cap)), LEN((cap)));		\
+    disk->member[LEN((cap))] = 0;				\
+}while(0)
+
+#define EXACTLY(cap, string)				\
+    (CAPTURED((cap)) &&					\
+     LEN((cap))==strlen((string)) &&			\
+     !memcmp(TEXT((cap)), (string), LEN((cap))))
+
+#define ERR(cap, msg) \
+    err_core(dpc, (msg), TEXT((cap)), LEN((cap)))
+
+    /* actually process the four capturing parens in our regexp */
+
+    STORE(1, pdev_path);
+
+    STORE(2, vdev);
+
+    if (!CAPTURED(3) || EXACTLY(3, ":")) {
+	disk->is_cdrom = 0;
+    } else if (EXACTLY(3, ":cdrom")) {
+	disk->is_cdrom = 1;
+    } else {
+	ERR(3, "unknown type (only `:' and `:cdrom' supported)");
+    }
+
+    if (!CAPTURED(4) || EXACTLY(4, ",w") || EXACTLY(4, ",")) {
+	disk->readwrite = 1;
+    } else if (EXACTLY(4, ",r")) {
+	disk->readwrite = 0;
+    } else {
+	ERR(4, "unknown read/write attribute");
+    }
+}
+
+int xlu_disk_parse(XLU_Config *cfg, const char *spec,
+		   libxl_device_disk *disk) {
+    YY_BUFFER_STATE buf = 0;
+    DiskParseContext dpc;
+    int e, r;
+
+    dpc.disk = disk;
+    dpc.spec = spec;
+    dpc.ctx.scanner = 0;
+
+    e = xlu__scanner_string_prep(&dpc.ctx,&buf, cfg,spec,strlen(spec));
+    if (e) { dpc.ctx.err = e; goto xe; }
+
+    r = xlu__disk_yylex(dpc.ctx.scanner);
+    if (r) assert(dpc.ctx.err);
+    if (dpc.ctx.err) goto xe;
+
+    if (disk->format == DISK_FORMAT_UNKNOWN) disk->format = DISK_FORMAT_RAW;
+
+ xe:
+    xlu__scanner_string_dispose(&dpc.ctx,&buf);
+    return dpc.ctx.err;
+}
+
diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
new file mode 100644
index 0000000..deb429f
--- /dev/null
+++ b/tools/libxl/libxlu_disk_i.h
@@ -0,0 +1,19 @@
+#ifndef LIBXLU_DISK_I_H
+#define LIBXLU_DISK_I_H
+
+#include "libxlu_internal.h"
+#include "libxl_internal.h"
+
+
+typedef struct {
+    CfgParseContext ctx;
+    const char *spec;
+    libxl_device_disk *disk;
+} DiskParseContext;
+
+void xlu__disk_err(DiskParseContext *dpc, const char *message);
+
+void xlu__disk_rhs(DiskParseContext *dpc);
+
+
+#endif /*LIBXLU_DISK_I_H*/
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
new file mode 100644
index 0000000..82e2248
--- /dev/null
+++ b/tools/libxl/libxlu_disk_l.l
@@ -0,0 +1,54 @@
+/* -*- fundamental -*- */
+
+%{
+#include "libxlu_disk_i.h"
+
+#define dpc ((DiskParseContext*)yyextra)
+#define YY_NO_INPUT
+
+#define DSET(member,enumname,valname) do{				\
+	if (dpc->disk->member != DISK_##enumname##_UNKNOWN &&		\
+	    dpc->disk->member != DISK_##enumname##_##valname) {		\
+	    xlu__disk_err(dpc, TOSTRING(member) " respecified");	\
+	    return 0;							\
+	} else {							\
+	    dpc->disk->member = DISK_##enumname##_##valname;		\
+	}								\
+    }while(0)
+
+/* Some versions of flex have a bug (Fedora bugzilla 612465) which causes
+ * it to fail to declare these functions, which it defines.  So declare
+ * them ourselves.  Hopefully we won't have to simultaneously support
+ * a flex version which declares these differently somehow. */
+int xlu__disk_yyget_column(yyscan_t yyscanner);
+void xlu__disk_yyset_column(int  column_no, yyscan_t yyscanner);
+
+%}
+
+%option warn
+%option nodefault
+%option batch
+%option 8bit
+%option noyywrap
+%option reentrant
+%option prefix="xlu__disk_yy"
+%option nounput
+
+%%
+
+raw:		{ DSET(format,FORMAT,RAW); }
+qcow:		{ DSET(format,FORMAT,QCOW); }
+qcow2:		{ DSET(format,FORMAT,QCOW2); }
+vhd:		{ DSET(format,FORMAT,QCOW2); }
+
+phy:		{ DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
+file:		{ DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
+tapdisk:|tap2?:	{ DSET(backend,BACKEND,TAP); }
+aio:		{ }
+ioemu:		{ }
+
+[a-z][a-z0-9-]*: { xlu__disk_err(dpc,"unknown disk format/method"); return 0; }
+
+[^:]*(\/.*)?	{ xlu__disk_rhs(dpc); return 0; }
+
+.*		{ xlu__disk_err(dpc,"bad disk syntax"); return 0; }
diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h
index e251a63..d681fba 100644
--- a/tools/libxl/libxlu_internal.h
+++ b/tools/libxl/libxlu_internal.h
@@ -21,10 +21,15 @@
 #include <string.h>
 #include <assert.h>
 
+#include <pcre.h>
+
 #define XLU_ConfigList XLU_ConfigSetting
 
 #include "libxlutil.h"
 
+
+
+
 struct XLU_ConfigSetting { /* transparent */
     struct XLU_ConfigSetting *next;
     char *name;
@@ -37,12 +42,27 @@ struct XLU_Config {
     XLU_ConfigSetting *settings;
     FILE *report;
     char *filename;
+    pcre *disk_re;
 };
 
 typedef struct {
     XLU_Config *cfg;
     int err, lexerrlineno, likely_python;
     void *scanner;
+    pcre *disk_re;
 } CfgParseContext;
 
+
+#ifndef YY_TYPEDEF_YY_BUFFER_STATE
+#define YY_TYPEDEF_YY_BUFFER_STATE
+typedef struct yy_buffer_state *YY_BUFFER_STATE;
+#endif
+
+int xlu__scanner_string_prep(CfgParseContext *ctx, YY_BUFFER_STATE *buf,
+			     XLU_Config *cfg, const char *data, int length);
+  /* Caller must initialise ctx->scanner=0 and buf=0 beforehand. */
+
+void xlu__scanner_string_dispose(CfgParseContext *ctx, YY_BUFFER_STATE *buf);
+
+
 #endif /*LIBXLU_INTERNAL_H*/
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index 8a6fcbd..acf96fd 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -58,4 +58,16 @@ const char *xlu_cfg_get_listitem(const XLU_ConfigList*, int entry);
   /* xlu_cfg_get_listitem cannot fail, except that if entry is
    * out of range it returns 0 (not setting errno) */
 
+
+/*
+ * Disk specification parsing.
+ */
+
+int xlu_disk_parse(XLU_Config *cfg, const char *spec,
+		   libxl_device_disk *disk);
+  /* disk must have been initialised; on error, returns errno value;
+   * bad strings, returns EINVAL and prints a message to cfg's report.
+   * That's all cfg is used for. */
+
+
 #endif /* LIBXLUTIL_H */
-- 
1.5.6.5

[PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse

[Ian Jackson]

From: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 tools/libxl/xl_cmdimpl.c |  160 +++++-----------------------------------------
 1 files changed, 16 insertions(+), 144 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 121a586..416de46 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -465,148 +465,22 @@ static int parse_action_on_shutdown(const char *buf, enum libxl_action_on_shutdo
 #define DSTATE_RW        5
 #define DSTATE_TERMINAL  6
 
-static int parse_disk_config(libxl_device_disk *disk, char *buf2)
+static void parse_disk_config(XLU_Config **config,
+			      libxl_device_disk *disk, const char *spec)
 {
-    int state = DSTATE_INITIAL;
-    char *p, *end, *tok;
-
-    memset(disk, 0, sizeof(*disk));
-
-    for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
-        switch(state){
-        case DSTATE_INITIAL:
-            if ( *p == ':' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "phy") ) {
-                    state = DSTATE_PHYSPATH;
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_PHY;
-                }else if ( !strcmp(tok, "file") ) {
-                    state = DSTATE_PHYSPATH;
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_TAP;
-                }else if ((!strcmp(tok, "tap")) ||
-                          (!strcmp(tok, "tap2"))) {
-                    state = DSTATE_TAP;
-                }else{
-                    fprintf(stderr, "Unknown disk type: %s\n", tok);
-                    return 0;
-                }
-                tok = p + 1;
-            } else if (*p == ',') {
-                state = DSTATE_VIRTPATH;
-                disk->format = DISK_FORMAT_EMPTY;
-                disk->backend = DISK_BACKEND_TAP;
-                disk->pdev_path = strdup("");
-                tok = p + 1;
-            }
-            break;
-        case DSTATE_TAP:
-            if (*p == ',') {
-                disk->format = DISK_FORMAT_RAW;
-                disk->backend = DISK_BACKEND_TAP;
-                state = DSTATE_PHYSPATH;
-            } else if ( *p == ':' ) {
-                *p = '\0';
-                if (!strcmp(tok, "aio")) {
-                    tok = p + 1;
-                    break;
-                }
-                if (!strcmp(tok, "vhd")) {
-                    disk->format = DISK_FORMAT_VHD;
-                    disk->backend = DISK_BACKEND_TAP;
-                }else if ( !strcmp(tok, "qcow") ) {
-                    disk->format = DISK_FORMAT_QCOW;
-                    disk->backend = DISK_BACKEND_QDISK;
-                }else if ( !strcmp(tok, "qcow2") ) {
-                    disk->format = DISK_FORMAT_QCOW2;
-                    disk->backend = DISK_BACKEND_QDISK;
-                }else if (!strcmp(tok, "raw")) {
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_TAP;
-                }
-                else {
-                    fprintf(stderr, "Unknown tapdisk type: %s\n", tok);
-                    return 0;
-                }
+    int e;
 
-                tok = p + 1;
-                state = DSTATE_PHYSPATH;
-                break;
-            } else {
-                break;
-            }
-        case DSTATE_PHYSPATH:
-            if ( *p == ',' ) {
-                int ioemu_len;
-
-                *p = '\0';
-                disk->pdev_path = (*tok) ? strdup(tok) : NULL;
-                tok = p + 1;
-
-                /* hack for ioemu disk spec */
-                ioemu_len = strlen("ioemu:");
-                state = DSTATE_VIRTPATH;
-                if ( tok + ioemu_len < end &&
-                    !strncmp(tok, "ioemu:", ioemu_len)) {
-                    tok += ioemu_len;
-                    p += ioemu_len;
-                }
-            }
-            break;
-        case DSTATE_VIRTPATH:
-            if ( *p == ',' || *p == ':' || *p == '\0' ) {
-                switch(*p) {
-                case ':':
-                    state = DSTATE_VIRTTYPE;
-                    break;
-                case ',':
-                    state = DSTATE_RW;
-                    break;
-                case '\0':
-                    state = DSTATE_TERMINAL;
-                    break;
-                }
-                if ( tok == p )
-                    goto out;
-                *p = '\0';
-                disk->vdev = (*tok) ? strdup(tok) : NULL;
-                tok = p + 1;
-            }
-            break;
-        case DSTATE_VIRTTYPE:
-            if ( *p == ',' || *p == '\0' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "cdrom") ) {
-                    disk->is_cdrom = 1;
-                    disk->unpluggable = 1;
-                }else{
-                    fprintf(stderr, "Unknown virtual disk type: %s\n", tok);
-                    return 0;
-                }
-                tok = p + 1;
-                state = (*p == ',') ? DSTATE_RW : DSTATE_TERMINAL;
-            }
-            break;
-        case DSTATE_RW:
-            if ( *p == '\0' ) {
-                disk->readwrite = (tok[0] == 'w');
-                tok = p + 1;
-                state = DSTATE_TERMINAL;
-            }
-            break;
-        case DSTATE_TERMINAL:
-            goto out;
-        }
+    if (!*config) {
+	*config = xlu_cfg_init(stderr, "command line");
+	if (!*config) { perror("xlu_cfg_init"); exit(-1); }
     }
 
-out:
-    if ( tok != p || state != DSTATE_TERMINAL ) {
-        fprintf(stderr, "parse error in disk config near '%s'\n", tok);
-        return 0;
+    e = xlu_disk_parse(*config, spec, disk);
+    if (e == EINVAL) exit(-1);
+    if (e) {
+	fprintf(stderr,"xlu_disk_parse failed: %s\n",strerror(errno));
+	exit(-1);
     }
-
-    return 1;
 }
 
 static void parse_config_data(const char *configfile_filename_report,
@@ -800,9 +674,7 @@ static void parse_config_data(const char *configfile_filename_report,
 
             d_config->disks = (libxl_device_disk *) realloc(d_config->disks, sizeof (libxl_device_disk) * (d_config->num_disks + 1));
             disk = d_config->disks + d_config->num_disks;
-            if ( !parse_disk_config(disk, buf2) ) {
-                exit(1);
-            }
+            parse_disk_config(&config, disk, buf2);
 
             free(buf2);
             d_config->num_disks++;
@@ -1873,6 +1745,7 @@ static void cd_insert(const char *dom, const char *virtdev, char *phys)
 {
     libxl_device_disk disk; /* we don't free disk's contents */
     char *buf = NULL;
+    XLU_Config *config;
 
     find_domain(dom);
 
@@ -1880,10 +1753,9 @@ static void cd_insert(const char *dom, const char *virtdev, char *phys)
         fprintf(stderr, "out of memory\n");
         return;
     }
-    if (!parse_disk_config(&disk, buf)) {
-        fprintf(stderr, "format error\n");
-        return;
-    }
+
+    parse_disk_config(&config, &disk, buf);
+
     disk.backend_domid = 0;
     disk.domid = domid;
 
-- 
1.5.6.5

[PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk

[Ian Jackson]

From: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 tools/libxl/xl_cmdimpl.c |   37 ++-----------------------------------
 1 files changed, 2 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 416de46..b1bc724 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4281,9 +4281,9 @@ int main_networkdetach(int argc, char **argv)
 int main_blockattach(int argc, char **argv)
 {
     int opt;
-    const char *tok;
     uint32_t fe_domid, be_domid = 0;
     libxl_device_disk disk = { 0 };
+    XLU_Config *config;
 
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
@@ -4300,40 +4300,7 @@ int main_blockattach(int argc, char **argv)
         return 0;
     }
 
-    tok = strtok(argv[optind+1], ":");
-    if (!strcmp(tok, "phy")) {
-        disk.backend = DISK_BACKEND_PHY;
-    } else if (!strcmp(tok, "file")) {
-        disk.backend = DISK_BACKEND_TAP;
-    } else if (!strcmp(tok, "tap")) {
-        tok = strtok(NULL, ":");
-        if (!strcmp(tok, "aio")) {
-            disk.backend = DISK_BACKEND_TAP;
-        } else if (!strcmp(tok, "vhd")) {
-            disk.format = DISK_FORMAT_VHD;
-            disk.backend = DISK_BACKEND_TAP;
-        } else if (!strcmp(tok, "qcow")) {
-            disk.format = DISK_FORMAT_QCOW;
-            disk.backend = DISK_BACKEND_QDISK;
-        } else if (!strcmp(tok, "qcow2")) {
-            disk.format = DISK_FORMAT_QCOW2;
-            disk.backend = DISK_BACKEND_QDISK;
-        } else {
-            fprintf(stderr, "Error: `%s' is not a valid disk image.\n", tok);
-            return 1;
-        }
-    } else {
-        fprintf(stderr, "Error: `%s' is not a valid block device.\n", tok);
-        return 1;
-    }
-    disk.pdev_path = strtok(NULL, "\0");
-    if (!disk.pdev_path) {
-        fprintf(stderr, "Error: missing path to disk image.\n");
-        return 1;
-    }
-    disk.vdev = argv[optind+2];
-    disk.unpluggable = 1;
-    disk.readwrite = ((argc-optind <= 3) || (argv[optind+3][0] == 'w'));
+    parse_disk_config(&config, &disk, argv[optind+1]);
 
     if (domain_qualifier_to_domid(argv[optind], &fe_domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
-- 
1.5.6.5

Re: [PATCH] libxl: new xlu_disk_parse function

[Gianni Tedesco]

On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> From: Ian Jackson <ijackson@chiark.greenend.org.uk>
> 
> Introduce new flex/regexp-based parser for disk configuration strings.
> 
> Callers will be updated in following patches.
> 
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
>  config/StdGNU.mk              |    1 +
>  tools/libxl/Makefile          |    9 +-
>  tools/libxl/libxlu_cfg.c      |   42 +++++++---
>  tools/libxl/libxlu_disk.c     |  164 +++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxlu_disk_i.h   |   19 +++++
>  tools/libxl/libxlu_disk_l.l   |   54 ++++++++++++++
>  tools/libxl/libxlu_internal.h |   20 +++++
>  tools/libxl/libxlutil.h       |   12 +++
>  8 files changed, 304 insertions(+), 17 deletions(-)
>  create mode 100644 tools/libxl/libxlu_disk.c
>  create mode 100644 tools/libxl/libxlu_disk_i.h
>  create mode 100644 tools/libxl/libxlu_disk_l.l
> 
> diff --git a/config/StdGNU.mk b/config/StdGNU.mk
> index 25aeb4d..9b331f0 100644
> --- a/config/StdGNU.mk
> +++ b/config/StdGNU.mk
> @@ -67,6 +67,7 @@ CURSES_LIBS = -lncurses
>  PTHREAD_LIBS = -lpthread
>  UTIL_LIBS = -lutil
>  DLOPEN_LIBS = -ldl
> +PCRE_LIBS = -lpcre
> 
>  SONAME_LDFLAG = -soname
>  SHLIB_LDFLAGS = -shared
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index a7b1d51..f0c473f 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -22,7 +22,7 @@ endif
>  LIBXL_LIBS =
>  LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
> 
> -LIBXLU_LIBS =
> +LIBXLU_LIBS = $(PCRE_LIBS)
> 
>  LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
>  ifeq ($(LIBXL_BLKTAP),y)
> @@ -38,9 +38,10 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
>                         libxl_internal.o libxl_utils.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += _libxl_types.o
> 
> -AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h
> -AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
> -LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o
> +AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h libxlu_disk_l.h
> +AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c libxlu_disk_l.c
> +LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> +       libxlu_disk_l.o libxlu_disk.o
> 
>  CLIENTS = xl
> 
> diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> index f947c21..fd3e102 100644
> --- a/tools/libxl/libxlu_cfg.c
> +++ b/tools/libxl/libxlu_cfg.c
> @@ -15,6 +15,7 @@ XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename) {
>      if (!cfg->filename) { free(cfg); return 0; }
> 
>      cfg->settings= 0;
> +    cfg->disk_re= 0;
>      return cfg;
>  }
> 
> @@ -38,6 +39,29 @@ static int ctx_prep(CfgParseContext *ctx, XLU_Config *cfg) {
> 
>  static void ctx_dispose(CfgParseContext *ctx) {
>      if (ctx->scanner) xlu__cfg_yylex_destroy(ctx->scanner);
> +    if (ctx->disk_re) pcre_free(ctx->disk_re);
> +}
> +
> +int xlu__scanner_string_prep(CfgParseContext *ctx, YY_BUFFER_STATE *buf,
> +                            XLU_Config *cfg, const char *data, int length) {
> +    int e;
> +
> +    e= ctx_prep(ctx, cfg);
> +    if (e) return e;
> +
> +    *buf = xlu__cfg_yy_scan_bytes(data, length, ctx->scanner);
> +    if (!*buf) {
> +        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
> +                cfg->filename);
> +        return ENOMEM;
> +    }
> +
> +    return 0;
> +}
> +
> +void xlu__scanner_string_dispose(CfgParseContext *ctx, YY_BUFFER_STATE *buf) {
> +    if (*buf) xlu__cfg_yy_delete_buffer(*buf, ctx->scanner);
> +    ctx_dispose(ctx);
>  }
> 
>  static void parse(CfgParseContext *ctx) {
> @@ -87,25 +111,17 @@ int xlu_cfg_readfile(XLU_Config *cfg, const char *real_filename) {
> 
>  int xlu_cfg_readdata(XLU_Config *cfg, const char *data, int length) {
>      int e;
> -    YY_BUFFER_STATE buf= 0;
> -
> +    YY_BUFFER_STATE buf=0;
>      CfgParseContext ctx;
> -    e= ctx_prep(&ctx, cfg);
> -    if (e) { ctx.err= e; goto xe; }
> +    ctx.scanner=0;
> 
> -    buf = xlu__cfg_yy_scan_bytes(data, length, ctx.scanner);
> -    if (!buf) {
> -        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
> -                cfg->filename);
> -        ctx.err= ENOMEM;
> -        goto xe;
> -    }
> +    e = xlu__scanner_string_prep(&ctx,&buf, cfg,data,length);
> +    if (e) { ctx.err = e; goto xe; }
> 
>      parse(&ctx);
> 
>   xe:
> -    if (buf) xlu__cfg_yy_delete_buffer(buf, ctx.scanner);
> -    ctx_dispose(&ctx);
> +    xlu__scanner_string_dispose(&ctx,&buf);
> 
>      return ctx.err;
>  }
> diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> new file mode 100644
> index 0000000..be523f7
> --- /dev/null
> +++ b/tools/libxl/libxlu_disk.c
> @@ -0,0 +1,164 @@
> +
> +/* Parsing the disk spec a tricky problem, because the target
> + * string might contain "," which is the delimiter we use for
> + * stripping off things on the RHS, and ":", which is the delimiter
> + * we use for stripping off things on the LHS.
> + *
> + * For the LHS we use a flex scanner, see libxlu_disk_l.l.
> + * That chops prefixes like "phy:" from the front of the string,
> + * accumulating information into the disk structure.
> + *
> + * For the RHS, we use a regexp using pcre.
> + */
> +
> +#include "libxlu_internal.h"
> +#include "libxlu_disk_l.h"
> +#include "libxlu_disk_i.h"
> +#include "libxlu_cfg_i.h"
> +
> +static void err_core(DiskParseContext *dpc, const char *message,
> +                    const char *in, int inlen) {
> +    fprintf(dpc->ctx.cfg->report,
> +           "%s: config parsing error in disk specification:"
> +           " %s: near `%s' in `%.*s'\n",
> +           dpc->ctx.cfg->filename, message, dpc->spec, inlen, in);
> +    if (!dpc->ctx.err) dpc->ctx.err= EINVAL;
> +}
> +
> +void xlu__disk_err(DiskParseContext *dpc, const char *message) {
> +    err_core(dpc, message,
> +            xlu__disk_yyget_text(dpc->ctx.scanner),
> +            xlu__disk_yyget_leng(dpc->ctx.scanner));
> +}
> +
> +void xlu__disk_rhs(DiskParseContext *dpc) {
> +
> +    /* variables used by pcre */
> +#define WORKSPACE_SIZE 20 /* wtf no manifest constant from pcre */
> +#define OVECTOR_SIZE 10
> +    int ovector[OVECTOR_SIZE];
> +    int workspace[WORKSPACE_SIZE];
> +
> +    int rc, len;
> +    const char *text;
> +    libxl_device_disk *disk = dpc->disk;
> +
> +    static const char *re_text=
> +       "(.*),(?:/dev/)?([0-9a-zA-Z]*)(:[0-9a-z]+)?(,[0-9a-z]*)?"
> +       /* 1:target     2:vdev         3:type        4:attrib (r/w) */;
> +
> +    /* compile the regexp if we haven't got one already */
> +    if (!dpc->ctx.disk_re) {
> +       const char *re_errstr;
> +       int re_errcode, re_erroffset;
> +
> +       dpc->ctx.disk_re = pcre_compile2(re_text, PCRE_DOTALL,
> +                                        &re_errcode, &re_errstr, &re_erroffset,
> +                                        0);
> +       if (!dpc->ctx.disk_re) {
> +           if (re_errcode == 21 /* wtf, no manifest constant */) {
> +               dpc->ctx.err = ENOMEM;
> +               return;
> +           }
> +           fprintf(dpc->ctx.cfg->report, "config parsing:"
> +                   " unable to compile regexp: %s [%d] (at `%s')",
> +                   re_errstr, re_errcode, re_text+re_erroffset);
> +           dpc->ctx.err = EIO;
> +           return;
> +       }
> +    }
> +
> +    /* actually run the regexp match */
> +
> +    text = xlu__disk_yyget_text(dpc->ctx.scanner);
> +    len = xlu__disk_yyget_leng(dpc->ctx.scanner);
> +
> +    rc = pcre_dfa_exec(dpc->ctx.disk_re, 0,
> +                      text, len, 0,
> +                      PCRE_ANCHORED,
> +                      ovector, OVECTOR_SIZE,
> +                      workspace, WORKSPACE_SIZE);
> +    switch (rc) {
> +    case PCRE_ERROR_NOMATCH:
> +       xlu__disk_err(dpc, "bad syntax for target, or missing vdev");
> +       return;

       case 1: ??

> +    case 2:
> +    case 3:
> +    case 4:
> +       break;

Or does it belong here? In which case aborting on a parse error is bad
juju.
       case 1:

> +    default:
> +       abort();
> +    }
> +
> +    /* macros for processing info from capturing parens; see pcreapi(3) */
> +
> +#define CAPTURED(cap) (START((cap)) >= 0)
> +#define START(cap) (ovector[(cap)*2])
> +#define END(cap)   (ovector[(cap)*2+1])
> +#define LEN(cap)   (END((cap)) - START((cap)))
> +#define TEXT(cap)  (text + START((cap)))
> +
> +#define STORE(cap, member) do{                                 \
> +    assert(CAPTURED((cap)));                                   \
> +    free(disk->member);                                                \
> +    disk->member = malloc(LEN((cap)) + 1);                     \
> +    if (!disk->member) { dpc->ctx.err = ENOMEM; return; }      \
> +    memcpy(disk->member, TEXT((cap)), LEN((cap)));             \
> +    disk->member[LEN((cap))] = 0;                              \
> +}while(0)
> +
> +#define EXACTLY(cap, string)                           \
> +    (CAPTURED((cap)) &&                                        \
> +     LEN((cap))==strlen((string)) &&                   \
> +     !memcmp(TEXT((cap)), (string), LEN((cap))))
> +
> +#define ERR(cap, msg) \
> +    err_core(dpc, (msg), TEXT((cap)), LEN((cap)))
> +
> +    /* actually process the four capturing parens in our regexp */
> +
> +    STORE(1, pdev_path);
> +
> +    STORE(2, vdev);
> +
> +    if (!CAPTURED(3) || EXACTLY(3, ":")) {
> +       disk->is_cdrom = 0;
> +    } else if (EXACTLY(3, ":cdrom")) {
> +       disk->is_cdrom = 1;
> +    } else {
> +       ERR(3, "unknown type (only `:' and `:cdrom' supported)");
> +    }
> +
> +    if (!CAPTURED(4) || EXACTLY(4, ",w") || EXACTLY(4, ",")) {
> +       disk->readwrite = 1;
> +    } else if (EXACTLY(4, ",r")) {
> +       disk->readwrite = 0;
> +    } else {
> +       ERR(4, "unknown read/write attribute");
> +    }
> +}

Hmm, I'm not sure this is nicer than the code it's replacing, it's
certainly a lot longer. I don't like the idea of it being full of things
comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
evaluating what the tokens are separately.

> +int xlu_disk_parse(XLU_Config *cfg, const char *spec,
> +                  libxl_device_disk *disk) {
> +    YY_BUFFER_STATE buf = 0;
> +    DiskParseContext dpc;
> +    int e, r;
> +
> +    dpc.disk = disk;
> +    dpc.spec = spec;
> +    dpc.ctx.scanner = 0;
> +
> +    e = xlu__scanner_string_prep(&dpc.ctx,&buf, cfg,spec,strlen(spec));
> +    if (e) { dpc.ctx.err = e; goto xe; }
> +
> +    r = xlu__disk_yylex(dpc.ctx.scanner);
> +    if (r) assert(dpc.ctx.err);
> +    if (dpc.ctx.err) goto xe;
> +
> +    if (disk->format == DISK_FORMAT_UNKNOWN) disk->format = DISK_FORMAT_RAW;
> +
> + xe:
> +    xlu__scanner_string_dispose(&dpc.ctx,&buf);
> +    return dpc.ctx.err;
> +}
> +
> diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
> new file mode 100644
> index 0000000..deb429f
> --- /dev/null
> +++ b/tools/libxl/libxlu_disk_i.h
> @@ -0,0 +1,19 @@
> +#ifndef LIBXLU_DISK_I_H
> +#define LIBXLU_DISK_I_H
> +
> +#include "libxlu_internal.h"
> +#include "libxl_internal.h"
> +
> +
> +typedef struct {
> +    CfgParseContext ctx;
> +    const char *spec;
> +    libxl_device_disk *disk;
> +} DiskParseContext;
> +
> +void xlu__disk_err(DiskParseContext *dpc, const char *message);
> +
> +void xlu__disk_rhs(DiskParseContext *dpc);
> +
> +
> +#endif /*LIBXLU_DISK_I_H*/
> diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
> new file mode 100644
> index 0000000..82e2248
> --- /dev/null
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -0,0 +1,54 @@
> +/* -*- fundamental -*- */
> +
> +%{
> +#include "libxlu_disk_i.h"
> +
> +#define dpc ((DiskParseContext*)yyextra)
> +#define YY_NO_INPUT
> +
> +#define DSET(member,enumname,valname) do{                              \
> +       if (dpc->disk->member != DISK_##enumname##_UNKNOWN &&           \
> +           dpc->disk->member != DISK_##enumname##_##valname) {         \
> +           xlu__disk_err(dpc, TOSTRING(member) " respecified");        \
> +           return 0;                                                   \
> +       } else {                                                        \
> +           dpc->disk->member = DISK_##enumname##_##valname;            \
> +       }                                                               \
> +    }while(0)
> +
> +/* Some versions of flex have a bug (Fedora bugzilla 612465) which causes
> + * it to fail to declare these functions, which it defines.  So declare
> + * them ourselves.  Hopefully we won't have to simultaneously support
> + * a flex version which declares these differently somehow. */
> +int xlu__disk_yyget_column(yyscan_t yyscanner);
> +void xlu__disk_yyset_column(int  column_no, yyscan_t yyscanner);
> +
> +%}
> +
> +%option warn
> +%option nodefault
> +%option batch
> +%option 8bit
> +%option noyywrap
> +%option reentrant
> +%option prefix="xlu__disk_yy"
> +%option nounput
> +
> +%%
> +
> +raw:           { DSET(format,FORMAT,RAW); }
> +qcow:          { DSET(format,FORMAT,QCOW); }
> +qcow2:         { DSET(format,FORMAT,QCOW2); }
> +vhd:           { DSET(format,FORMAT,QCOW2); }
> +
> +phy:           { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
> +file:          { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
> +tapdisk:|tap2?:        { DSET(backend,BACKEND,TAP); }
> +aio:           { }
> +ioemu:         { }

This bit is quite nice though. We could probably just tidy up the
existing parser using arrays of values and things rather than a lot of
if/else statements though.

> +[a-z][a-z0-9-]*: { xlu__disk_err(dpc,"unknown disk format/method"); return 0; }
> +
> +[^:]*(\/.*)?   { xlu__disk_rhs(dpc); return 0; }
> +
> +.*             { xlu__disk_err(dpc,"bad disk syntax"); return 0; }
> diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h
> index e251a63..d681fba 100644
> --- a/tools/libxl/libxlu_internal.h
> +++ b/tools/libxl/libxlu_internal.h
> @@ -21,10 +21,15 @@
>  #include <string.h>
>  #include <assert.h>
> 
> +#include <pcre.h>
> +
>  #define XLU_ConfigList XLU_ConfigSetting
> 
>  #include "libxlutil.h"
> 
> +
> +
> +
>  struct XLU_ConfigSetting { /* transparent */
>      struct XLU_ConfigSetting *next;
>      char *name;
> @@ -37,12 +42,27 @@ struct XLU_Config {
>      XLU_ConfigSetting *settings;
>      FILE *report;
>      char *filename;
> +    pcre *disk_re;
>  };
> 
>  typedef struct {
>      XLU_Config *cfg;
>      int err, lexerrlineno, likely_python;
>      void *scanner;
> +    pcre *disk_re;
>  } CfgParseContext;
> 
> +
> +#ifndef YY_TYPEDEF_YY_BUFFER_STATE
> +#define YY_TYPEDEF_YY_BUFFER_STATE
> +typedef struct yy_buffer_state *YY_BUFFER_STATE;
> +#endif
> +
> +int xlu__scanner_string_prep(CfgParseContext *ctx, YY_BUFFER_STATE *buf,
> +                            XLU_Config *cfg, const char *data, int length);
> +  /* Caller must initialise ctx->scanner=0 and buf=0 beforehand. */
> +
> +void xlu__scanner_string_dispose(CfgParseContext *ctx, YY_BUFFER_STATE *buf);
> +
> +
>  #endif /*LIBXLU_INTERNAL_H*/
> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
> index 8a6fcbd..acf96fd 100644
> --- a/tools/libxl/libxlutil.h
> +++ b/tools/libxl/libxlutil.h
> @@ -58,4 +58,16 @@ const char *xlu_cfg_get_listitem(const XLU_ConfigList*, int entry);
>    /* xlu_cfg_get_listitem cannot fail, except that if entry is
>     * out of range it returns 0 (not setting errno) */
> 
> +
> +/*
> + * Disk specification parsing.
> + */
> +
> +int xlu_disk_parse(XLU_Config *cfg, const char *spec,
> +                  libxl_device_disk *disk);
> +  /* disk must have been initialised; on error, returns errno value;
> +   * bad strings, returns EINVAL and prints a message to cfg's report.
> +   * That's all cfg is used for. */
> +
> +
>  #endif /* LIBXLUTIL_H */
> --
> 1.5.6.5

Gianni

Re: [PATCH] libxl: new xlu_disk_parse function

[Stefano Stabellini]

On Mon, 28 Mar 2011, Gianni Tedesco wrote:
> On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > From: Ian Jackson <ijackson@chiark.greenend.org.uk>
> >
> > Introduce new flex/regexp-based parser for disk configuration strings.
> >
> > Callers will be updated in following patches.
> >
> > Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> > ---
> >  config/StdGNU.mk              |    1 +
> >  tools/libxl/Makefile          |    9 +-
> >  tools/libxl/libxlu_cfg.c      |   42 +++++++---
> >  tools/libxl/libxlu_disk.c     |  164 +++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxlu_disk_i.h   |   19 +++++
> >  tools/libxl/libxlu_disk_l.l   |   54 ++++++++++++++
> >  tools/libxl/libxlu_internal.h |   20 +++++
> >  tools/libxl/libxlutil.h       |   12 +++
> >  8 files changed, 304 insertions(+), 17 deletions(-)
> >  create mode 100644 tools/libxl/libxlu_disk.c
> >  create mode 100644 tools/libxl/libxlu_disk_i.h
> >  create mode 100644 tools/libxl/libxlu_disk_l.l
> >
> > diff --git a/config/StdGNU.mk b/config/StdGNU.mk
> > index 25aeb4d..9b331f0 100644
> > --- a/config/StdGNU.mk
> > +++ b/config/StdGNU.mk
> > @@ -67,6 +67,7 @@ CURSES_LIBS = -lncurses
> >  PTHREAD_LIBS = -lpthread
> >  UTIL_LIBS = -lutil
> >  DLOPEN_LIBS = -ldl
> > +PCRE_LIBS = -lpcre
> >
> >  SONAME_LDFLAG = -soname
> >  SHLIB_LDFLAGS = -shared
> > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > index a7b1d51..f0c473f 100644
> > --- a/tools/libxl/Makefile
> > +++ b/tools/libxl/Makefile
> > @@ -22,7 +22,7 @@ endif
> >  LIBXL_LIBS =
> >  LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
> >
> > -LIBXLU_LIBS =
> > +LIBXLU_LIBS = $(PCRE_LIBS)
> >
> >  LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
> >  ifeq ($(LIBXL_BLKTAP),y)
> > @@ -38,9 +38,10 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
> >                         libxl_internal.o libxl_utils.o $(LIBXL_OBJS-y)
> >  LIBXL_OBJS += _libxl_types.o
> >
> > -AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h
> > -AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
> > -LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o
> > +AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h libxlu_disk_l.h
> > +AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c libxlu_disk_l.c
> > +LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> > +       libxlu_disk_l.o libxlu_disk.o
> >
> >  CLIENTS = xl
> >
> > diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> > index f947c21..fd3e102 100644
> > --- a/tools/libxl/libxlu_cfg.c
> > +++ b/tools/libxl/libxlu_cfg.c
> > @@ -15,6 +15,7 @@ XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename) {
> >      if (!cfg->filename) { free(cfg); return 0; }
> >
> >      cfg->settings= 0;
> > +    cfg->disk_re= 0;
> >      return cfg;
> >  }
> >
> > @@ -38,6 +39,29 @@ static int ctx_prep(CfgParseContext *ctx, XLU_Config *cfg) {
> >
> >  static void ctx_dispose(CfgParseContext *ctx) {
> >      if (ctx->scanner) xlu__cfg_yylex_destroy(ctx->scanner);
> > +    if (ctx->disk_re) pcre_free(ctx->disk_re);
> > +}
> > +
> > +int xlu__scanner_string_prep(CfgParseContext *ctx, YY_BUFFER_STATE *buf,
> > +                            XLU_Config *cfg, const char *data, int length) {
> > +    int e;
> > +
> > +    e= ctx_prep(ctx, cfg);
> > +    if (e) return e;
> > +
> > +    *buf = xlu__cfg_yy_scan_bytes(data, length, ctx->scanner);
> > +    if (!*buf) {
> > +        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
> > +                cfg->filename);
> > +        return ENOMEM;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +void xlu__scanner_string_dispose(CfgParseContext *ctx, YY_BUFFER_STATE *buf) {
> > +    if (*buf) xlu__cfg_yy_delete_buffer(*buf, ctx->scanner);
> > +    ctx_dispose(ctx);
> >  }
> >
> >  static void parse(CfgParseContext *ctx) {
> > @@ -87,25 +111,17 @@ int xlu_cfg_readfile(XLU_Config *cfg, const char *real_filename) {
> >
> >  int xlu_cfg_readdata(XLU_Config *cfg, const char *data, int length) {
> >      int e;
> > -    YY_BUFFER_STATE buf= 0;
> > -
> > +    YY_BUFFER_STATE buf=0;
> >      CfgParseContext ctx;
> > -    e= ctx_prep(&ctx, cfg);
> > -    if (e) { ctx.err= e; goto xe; }
> > +    ctx.scanner=0;
> >
> > -    buf = xlu__cfg_yy_scan_bytes(data, length, ctx.scanner);
> > -    if (!buf) {
> > -        fprintf(cfg->report,"%s: unable to allocate scanner buffer\n",
> > -                cfg->filename);
> > -        ctx.err= ENOMEM;
> > -        goto xe;
> > -    }
> > +    e = xlu__scanner_string_prep(&ctx,&buf, cfg,data,length);
> > +    if (e) { ctx.err = e; goto xe; }
> >
> >      parse(&ctx);
> >
> >   xe:
> > -    if (buf) xlu__cfg_yy_delete_buffer(buf, ctx.scanner);
> > -    ctx_dispose(&ctx);
> > +    xlu__scanner_string_dispose(&ctx,&buf);
> >
> >      return ctx.err;
> >  }
> > diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> > new file mode 100644
> > index 0000000..be523f7
> > --- /dev/null
> > +++ b/tools/libxl/libxlu_disk.c
> > @@ -0,0 +1,164 @@
> > +
> > +/* Parsing the disk spec a tricky problem, because the target
> > + * string might contain "," which is the delimiter we use for
> > + * stripping off things on the RHS, and ":", which is the delimiter
> > + * we use for stripping off things on the LHS.
> > + *
> > + * For the LHS we use a flex scanner, see libxlu_disk_l.l.
> > + * That chops prefixes like "phy:" from the front of the string,
> > + * accumulating information into the disk structure.
> > + *
> > + * For the RHS, we use a regexp using pcre.
> > + */
> > +
> > +#include "libxlu_internal.h"
> > +#include "libxlu_disk_l.h"
> > +#include "libxlu_disk_i.h"
> > +#include "libxlu_cfg_i.h"
> > +
> > +static void err_core(DiskParseContext *dpc, const char *message,
> > +                    const char *in, int inlen) {
> > +    fprintf(dpc->ctx.cfg->report,
> > +           "%s: config parsing error in disk specification:"
> > +           " %s: near `%s' in `%.*s'\n",
> > +           dpc->ctx.cfg->filename, message, dpc->spec, inlen, in);
> > +    if (!dpc->ctx.err) dpc->ctx.err= EINVAL;
> > +}
> > +
> > +void xlu__disk_err(DiskParseContext *dpc, const char *message) {
> > +    err_core(dpc, message,
> > +            xlu__disk_yyget_text(dpc->ctx.scanner),
> > +            xlu__disk_yyget_leng(dpc->ctx.scanner));
> > +}
> > +
> > +void xlu__disk_rhs(DiskParseContext *dpc) {
> > +
> > +    /* variables used by pcre */
> > +#define WORKSPACE_SIZE 20 /* wtf no manifest constant from pcre */
> > +#define OVECTOR_SIZE 10
> > +    int ovector[OVECTOR_SIZE];
> > +    int workspace[WORKSPACE_SIZE];
> > +
> > +    int rc, len;
> > +    const char *text;
> > +    libxl_device_disk *disk = dpc->disk;
> > +
> > +    static const char *re_text=
> > +       "(.*),(?:/dev/)?([0-9a-zA-Z]*)(:[0-9a-z]+)?(,[0-9a-z]*)?"
> > +       /* 1:target     2:vdev         3:type        4:attrib (r/w) */;
> > +
> > +    /* compile the regexp if we haven't got one already */
> > +    if (!dpc->ctx.disk_re) {
> > +       const char *re_errstr;
> > +       int re_errcode, re_erroffset;
> > +
> > +       dpc->ctx.disk_re = pcre_compile2(re_text, PCRE_DOTALL,
> > +                                        &re_errcode, &re_errstr, &re_erroffset,
> > +                                        0);
> > +       if (!dpc->ctx.disk_re) {
> > +           if (re_errcode == 21 /* wtf, no manifest constant */) {
> > +               dpc->ctx.err = ENOMEM;
> > +               return;
> > +           }
> > +           fprintf(dpc->ctx.cfg->report, "config parsing:"
> > +                   " unable to compile regexp: %s [%d] (at `%s')",
> > +                   re_errstr, re_errcode, re_text+re_erroffset);
> > +           dpc->ctx.err = EIO;
> > +           return;
> > +       }
> > +    }
> > +
> > +    /* actually run the regexp match */
> > +
> > +    text = xlu__disk_yyget_text(dpc->ctx.scanner);
> > +    len = xlu__disk_yyget_leng(dpc->ctx.scanner);
> > +
> > +    rc = pcre_dfa_exec(dpc->ctx.disk_re, 0,
> > +                      text, len, 0,
> > +                      PCRE_ANCHORED,
> > +                      ovector, OVECTOR_SIZE,
> > +                      workspace, WORKSPACE_SIZE);
> > +    switch (rc) {
> > +    case PCRE_ERROR_NOMATCH:
> > +       xlu__disk_err(dpc, "bad syntax for target, or missing vdev");
> > +       return;
> 
>        case 1: ??
> 
> > +    case 2:
> > +    case 3:
> > +    case 4:
> > +       break;
> 
> Or does it belong here? In which case aborting on a parse error is bad
> juju.
>        case 1:
> 
> > +    default:
> > +       abort();
> > +    }
> > +
> > +    /* macros for processing info from capturing parens; see pcreapi(3) */
> > +
> > +#define CAPTURED(cap) (START((cap)) >= 0)
> > +#define START(cap) (ovector[(cap)*2])
> > +#define END(cap)   (ovector[(cap)*2+1])
> > +#define LEN(cap)   (END((cap)) - START((cap)))
> > +#define TEXT(cap)  (text + START((cap)))
> > +
> > +#define STORE(cap, member) do{                                 \
> > +    assert(CAPTURED((cap)));                                   \
> > +    free(disk->member);                                                \
> > +    disk->member = malloc(LEN((cap)) + 1);                     \
> > +    if (!disk->member) { dpc->ctx.err = ENOMEM; return; }      \
> > +    memcpy(disk->member, TEXT((cap)), LEN((cap)));             \
> > +    disk->member[LEN((cap))] = 0;                              \
> > +}while(0)
> > +
> > +#define EXACTLY(cap, string)                           \
> > +    (CAPTURED((cap)) &&                                        \
> > +     LEN((cap))==strlen((string)) &&                   \
> > +     !memcmp(TEXT((cap)), (string), LEN((cap))))
> > +
> > +#define ERR(cap, msg) \
> > +    err_core(dpc, (msg), TEXT((cap)), LEN((cap)))
> > +
> > +    /* actually process the four capturing parens in our regexp */
> > +
> > +    STORE(1, pdev_path);
> > +
> > +    STORE(2, vdev);
> > +
> > +    if (!CAPTURED(3) || EXACTLY(3, ":")) {
> > +       disk->is_cdrom = 0;
> > +    } else if (EXACTLY(3, ":cdrom")) {
> > +       disk->is_cdrom = 1;
> > +    } else {
> > +       ERR(3, "unknown type (only `:' and `:cdrom' supported)");
> > +    }
> > +
> > +    if (!CAPTURED(4) || EXACTLY(4, ",w") || EXACTLY(4, ",")) {
> > +       disk->readwrite = 1;
> > +    } else if (EXACTLY(4, ",r")) {
> > +       disk->readwrite = 0;
> > +    } else {
> > +       ERR(4, "unknown read/write attribute");
> > +    }
> > +}
> 
> Hmm, I'm not sure this is nicer than the code it's replacing, it's
> certainly a lot longer. I don't like the idea of it being full of things
> comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> evaluating what the tokens are separately.

I definitely agree.
Besides when doing refactoring the code produced should be either shorter or
at least easier to read but this code is neither of them.

Re: [PATCH] libxl: new xlu_disk_parse function

[Ian Campbell]

Please trim your quotes guys!

On Mon, 2011-03-28 at 19:13 +0100, Stefano Stabellini wrote:
> On Mon, 28 Mar 2011, Gianni Tedesco wrote:
> Besides when doing refactoring the code produced should be either shorter or
> at least easier to read but this code is neither of them.

Without reference to this specific patchset I disagree with "shorter"
here. The important things are clarity, maintainability, correctness etc
(and possibly performance depending on circumstances).

Brevity is a poor indicator for any of these (or anything really, unless
you are playing Perl golf) although it may be a partial factor in some
of them.

It is perfectly possible that a refactoring doubles the number of lines
of code yet is still a clear improvement.

Ian.

Re: [PATCH] libxl: new xlu_disk_parse function

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> On Mon, 28 Mar 2011, Gianni Tedesco wrote:
> > Hmm, I'm not sure this is nicer than the code it's replacing, it's
> > certainly a lot longer. I don't like the idea of it being full of things
> > comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> > evaluating what the tokens are separately.
> 
> I definitely agree.
> Besides when doing refactoring the code produced should be either shorter or
> at least easier to read but this code is neither of them.

I agree that as a rule of thumb better code is shorter.  Unfortunately
string parsing in C is one of those exceptions.

What I was trying to do was to separate out the actual syntax in a way
that is reasonably tractable, and which is amenable to being edited
later without introducing bugs.

So while the current code is longer in this case I think it's clearer.
A good deal of the added code is very simple and straightforward "set
up this regexp engine and check the return value".

It is a shame that the syntax is so abstruse.  We can do better with
the nic parameters which have a better syntax.

Ian.

Re: [PATCH] libxl: new xlu_disk_parse function

[Stefano Stabellini]

On Thu, 31 Mar 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> > On Mon, 28 Mar 2011, Gianni Tedesco wrote:
> > > Hmm, I'm not sure this is nicer than the code it's replacing, it's
> > > certainly a lot longer. I don't like the idea of it being full of things
> > > comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> > > evaluating what the tokens are separately.
> > 
> > I definitely agree.
> > Besides when doing refactoring the code produced should be either shorter or
> > at least easier to read but this code is neither of them.
> 
> I agree that as a rule of thumb better code is shorter.  Unfortunately
> string parsing in C is one of those exceptions.
> 
> What I was trying to do was to separate out the actual syntax in a way
> that is reasonably tractable, and which is amenable to being edited
> later without introducing bugs.
> 
> So while the current code is longer in this case I think it's clearer.
> A good deal of the added code is very simple and straightforward "set
> up this regexp engine and check the return value".

The problem with this code is that, exactly like regex in general, is
write-only.
In one year time making a change to this code would be harder than
rewrite it again from scratch for the third time.
I vote for the explicit state machine.

Re: [PATCH] libxl: new xlu_disk_parse function

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > +       return;
> 
>        case 1: ??

Deliberately omitted.

> > +    case 2:
> > +    case 3:
> > +    case 4:
> > +       break;
> 
> Or does it belong here? In which case aborting on a parse error is bad
> juju.
>        case 1:
> > +    default:
> > +       abort();

I could add it there for clarity.  The regexp will always match
capturing with 2, 3 or 4 parens.  None of the other errors from
dfa_exec are applicable.  So anything other than 2,3,4 or "did not
match" is due to a bug in the code, not merely bogus input.  Perhaps
this should be mentioned in a comment.

> Hmm, I'm not sure this is nicer than the code it's replacing, it's
> certainly a lot longer. I don't like the idea of it being full of things
> comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> evaluating what the tokens are separately.

Perhaps you're right.  Unfortunately the nasty multi-level nature of
this parsing problem makes this a bit awkward.

But I think I can remove the delimiters into the regexp which perhaps
will help.

> > +raw:           { DSET(format,FORMAT,RAW); }
> > +qcow:          { DSET(format,FORMAT,QCOW); }
> > +qcow2:         { DSET(format,FORMAT,QCOW2); }
> > +vhd:           { DSET(format,FORMAT,QCOW2); }
> > +
> > +phy:           { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
> > +file:          { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
> > +tapdisk:|tap2?:        { DSET(backend,BACKEND,TAP); }
> > +aio:           { }
> > +ioemu:         { }
> 
> This bit is quite nice though. We could probably just tidy up the
> existing parser using arrays of values and things rather than a lot of
> if/else statements though.

I wanted to avoid parsing with pointer arithmetic, which is very easy
to write bugs in - particularly when new features are added.

Ian.

Re: [PATCH] libxl: new xlu_disk_parse function

[Ian Campbell]

On Thu, 2011-03-31 at 18:30 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> > On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > > +       return;
> > 
> >        case 1: ??
> 
> Deliberately omitted.
> 
> > > +    case 2:
> > > +    case 3:
> > > +    case 4:
> > > +       break;
> > 
> > Or does it belong here? In which case aborting on a parse error is bad
> > juju.
> >        case 1:
> > > +    default:
> > > +       abort();
> 
> I could add it there for clarity.  The regexp will always match
> capturing with 2, 3 or 4 parens.  None of the other errors from
> dfa_exec are applicable.  So anything other than 2,3,4 or "did not
> match" is due to a bug in the code, not merely bogus input.  Perhaps
> this should be mentioned in a comment.

I think that's a good idea, you'll only up nacking an endless stream of
fixup patches otherwise (inevitably adding the case in the wrong
place...).

IMHO a switch statement in this context obfuscates the error handling
and a couple of error handling if statements would be more obvious e.g.

	if (rc == PCRE_ERROR_NOMATCH) {
		xlu__disk_err(dpc, "bad syntax for target, or missing vdev");
		return;
	}

	/* The regexp will always match capturing with 2, 3 or 4 parens */
	if (rc < 2 || rc > 4)
		abort();

	.. carry on...

Re: [PATCH] libxl: new xlu_disk_parse function

[Stefano Stabellini]

On Thu, 31 Mar 2011, Ian Jackson wrote:
> > > +raw:           { DSET(format,FORMAT,RAW); }
> > > +qcow:          { DSET(format,FORMAT,QCOW); }
> > > +qcow2:         { DSET(format,FORMAT,QCOW2); }
> > > +vhd:           { DSET(format,FORMAT,QCOW2); }
> > > +
> > > +phy:           { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
> > > +file:          { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
> > > +tapdisk:|tap2?:        { DSET(backend,BACKEND,TAP); }
> > > +aio:           { }
> > > +ioemu:         { }
> > 
> > This bit is quite nice though. We could probably just tidy up the
> > existing parser using arrays of values and things rather than a lot of
> > if/else statements though.
> 
> I wanted to avoid parsing with pointer arithmetic, which is very easy
> to write bugs in - particularly when new features are added.

We'll just have to be careful. I certainly find easier to read (and
therefore to debug and maintain) the current state machine than this
patch.

Re: [PATCH] libxl: new xlu_disk_parse function

[Gianni Tedesco]

On Thu, 2011-03-31 at 18:30 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> > On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > > +       return;
> > 
> >        case 1: ??
> 
> Deliberately omitted.
> 
> > > +    case 2:
> > > +    case 3:
> > > +    case 4:
> > > +       break;
> > 
> > Or does it belong here? In which case aborting on a parse error is bad
> > juju.
> >        case 1:
> > > +    default:
> > > +       abort();
> 
> I could add it there for clarity.  The regexp will always match
> capturing with 2, 3 or 4 parens.  None of the other errors from
> dfa_exec are applicable.  So anything other than 2,3,4 or "did not
> match" is due to a bug in the code, not merely bogus input.  Perhaps
> this should be mentioned in a comment.
> 
> > Hmm, I'm not sure this is nicer than the code it's replacing, it's
> > certainly a lot longer. I don't like the idea of it being full of things
> > comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> > evaluating what the tokens are separately.
> 
> Perhaps you're right.  Unfortunately the nasty multi-level nature of
> this parsing problem makes this a bit awkward.
> 
> But I think I can remove the delimiters into the regexp which perhaps
> will help.
> 
> > > +raw:           { DSET(format,FORMAT,RAW); }
> > > +qcow:          { DSET(format,FORMAT,QCOW); }
> > > +qcow2:         { DSET(format,FORMAT,QCOW2); }
> > > +vhd:           { DSET(format,FORMAT,QCOW2); }
> > > +
> > > +phy:           { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
> > > +file:          { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
> > > +tapdisk:|tap2?:        { DSET(backend,BACKEND,TAP); }
> > > +aio:           { }
> > > +ioemu:         { }
> > 
> > This bit is quite nice though. We could probably just tidy up the
> > existing parser using arrays of values and things rather than a lot of
> > if/else statements though.
> 
> I wanted to avoid parsing with pointer arithmetic, which is very easy
> to write bugs in - particularly when new features are added.

Well it is designed so that the pointer arithmetic parts are the main
loop which doesn't need altering. Within the loop it's just a matter of
doing state transitions and that is the essence of the parser. Just a
bit of care and testing when modifying it is all that's required - it's
not *that* subtle.

Gianni