Ian Jackson
2011-Mar-24 17:49 UTC
[Xen-devel] [PATCH] libxl: move TOSTRING to libxl_internal.h
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-24 17:49 UTC
[Xen-devel] [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-24 17:49 UTC
[Xen-devel] [PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Mar-28 17:31 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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.5Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-28 18:13 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-29 09:10 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-31 17:30 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-31 17:33 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Apr-01 15:28 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Apr-02 08:51 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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... _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Apr-04 11:29 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Apr-14 07:20 UTC
Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel