* [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 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-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-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.