* [PATCH] libxl: move TOSTRING to libxl_internal.h @ 2011-03-24 17:49 Ian Jackson 2011-03-24 17:49 ` [PATCH] libxl: new xlu_disk_parse function Ian Jackson 0 siblings, 1 reply; 13+ messages in thread From: Ian Jackson @ 2011-03-24 17:49 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] libxl: new xlu_disk_parse function 2011-03-24 17:49 [PATCH] libxl: move TOSTRING to libxl_internal.h Ian Jackson @ 2011-03-24 17:49 ` Ian Jackson 2011-03-24 17:49 ` [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse Ian Jackson 2011-03-28 17:31 ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco 0 siblings, 2 replies; 13+ messages in thread From: Ian Jackson @ 2011-03-24 17:49 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse 2011-03-24 17:49 ` [PATCH] libxl: new xlu_disk_parse function Ian Jackson @ 2011-03-24 17:49 ` Ian Jackson 2011-03-24 17:49 ` [PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk Ian Jackson 2011-03-28 17:31 ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco 1 sibling, 1 reply; 13+ messages in thread From: Ian Jackson @ 2011-03-24 17:49 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk 2011-03-24 17:49 ` [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse Ian Jackson @ 2011-03-24 17:49 ` Ian Jackson 0 siblings, 0 replies; 13+ messages in thread From: Ian Jackson @ 2011-03-24 17:49 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-24 17:49 ` [PATCH] libxl: new xlu_disk_parse function Ian Jackson 2011-03-24 17:49 ` [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse Ian Jackson @ 2011-03-28 17:31 ` Gianni Tedesco 2011-03-28 18:13 ` Stefano Stabellini 2011-03-31 17:30 ` Ian Jackson 1 sibling, 2 replies; 13+ messages in thread From: Gianni Tedesco @ 2011-03-28 17:31 UTC (permalink / raw) To: Ian Jackson; +Cc: Ian, xen-devel, Jackson 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-28 17:31 ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco @ 2011-03-28 18:13 ` Stefano Stabellini 2011-03-29 9:10 ` Ian Campbell 2011-03-31 17:33 ` Ian Jackson 2011-03-31 17:30 ` Ian Jackson 1 sibling, 2 replies; 13+ messages in thread From: Stefano Stabellini @ 2011-03-28 18:13 UTC (permalink / raw) To: Gianni Tedesco; +Cc: Ian, xen-devel, Ian Jackson, Jackson 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-28 18:13 ` Stefano Stabellini @ 2011-03-29 9:10 ` Ian Campbell 2011-03-31 17:33 ` Ian Jackson 1 sibling, 0 replies; 13+ messages in thread From: Ian Campbell @ 2011-03-29 9:10 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson, Gianni Tedesco 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-28 18:13 ` Stefano Stabellini 2011-03-29 9:10 ` Ian Campbell @ 2011-03-31 17:33 ` Ian Jackson 2011-04-01 15:28 ` Stefano Stabellini 1 sibling, 1 reply; 13+ messages in thread From: Ian Jackson @ 2011-03-31 17:33 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, Gianni Tedesco 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-31 17:33 ` Ian Jackson @ 2011-04-01 15:28 ` Stefano Stabellini 0 siblings, 0 replies; 13+ messages in thread From: Stefano Stabellini @ 2011-04-01 15:28 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, Gianni Tedesco, 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-28 17:31 ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco 2011-03-28 18:13 ` Stefano Stabellini @ 2011-03-31 17:30 ` Ian Jackson 2011-04-02 8:51 ` Ian Campbell ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Ian Jackson @ 2011-03-31 17:30 UTC (permalink / raw) To: Gianni Tedesco; +Cc: xen-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-31 17:30 ` Ian Jackson @ 2011-04-02 8:51 ` Ian Campbell 2011-04-04 11:29 ` Stefano Stabellini 2011-04-14 7:20 ` Gianni Tedesco 2 siblings, 0 replies; 13+ messages in thread From: Ian Campbell @ 2011-04-02 8:51 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, 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. 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... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-31 17:30 ` Ian Jackson 2011-04-02 8:51 ` Ian Campbell @ 2011-04-04 11:29 ` Stefano Stabellini 2011-04-14 7:20 ` Gianni Tedesco 2 siblings, 0 replies; 13+ messages in thread From: Stefano Stabellini @ 2011-04-04 11:29 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, Gianni Tedesco 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libxl: new xlu_disk_parse function 2011-03-31 17:30 ` Ian Jackson 2011-04-02 8:51 ` Ian Campbell 2011-04-04 11:29 ` Stefano Stabellini @ 2011-04-14 7:20 ` Gianni Tedesco 2 siblings, 0 replies; 13+ messages in thread From: Gianni Tedesco @ 2011-04-14 7:20 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-04-14 7:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-24 17:49 [PATCH] libxl: move TOSTRING to libxl_internal.h Ian Jackson 2011-03-24 17:49 ` [PATCH] libxl: new xlu_disk_parse function Ian Jackson 2011-03-24 17:49 ` [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse Ian Jackson 2011-03-24 17:49 ` [PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk Ian Jackson 2011-03-28 17:31 ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco 2011-03-28 18:13 ` Stefano Stabellini 2011-03-29 9:10 ` Ian Campbell 2011-03-31 17:33 ` Ian Jackson 2011-04-01 15:28 ` Stefano Stabellini 2011-03-31 17:30 ` Ian Jackson 2011-04-02 8:51 ` Ian Campbell 2011-04-04 11:29 ` Stefano Stabellini 2011-04-14 7:20 ` Gianni Tedesco
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.