[libvirt] [PATCH 1/8] virsh: better support double quote
In origin code, double quote is only allowed at the begin or end "complicated argument" --some_opt="complicated string" (we split this argument into 2 parts, option and data, the data is "complicated string"). This patch makes it allow double quote at any position of an argument: complicated" argument" complicated" "argument --"some opt=complicated string" This patch is also needed for the following patches, the following patches will not split option argument into 2 parts, so we have to allow double quote at any position of an argument. Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- diff --git a/tools/virsh.c b/tools/virsh.c index 57ea618..7b6f2b6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10172,9 +10172,9 @@ static int vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) { int tk = VSH_TK_NONE; - int quote = FALSE; + bool double_quote = false; int sz = 0; - char *p = str; + char *p = str, *q; char *tkstr = NULL; *end = NULL; @@ -10188,9 +10188,13 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) *end = ++p; /* = \0 or begin of next command */ return VSH_TK_END; } + + q = p; + *res = NULL; +copy: while (*p) { /* end of token is blank space or ';' */ - if ((quote == FALSE && (*p == ' ' || *p == '\t')) || *p == ';') + if (!double_quote && (*p == ' ' || *p == '\t' || *p == ';')) break; /* end of option name could be '=' */ @@ -10206,23 +10210,22 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) p += 2; } else { tk = VSH_TK_DATA; - if (*p == '"') { - quote = TRUE; - p++; - } else { - quote = FALSE; - } } tkstr = p; /* begin of token */ - } else if (quote && *p == '"') { - quote = FALSE; + } + + if (*p == '"') { + double_quote = !double_quote; p++; - break; /* end of "..." token */ + continue; } + + if (*res) + (*res)[sz] = *p; p++; sz++; } - if (quote) { + if (double_quote) { vshError(ctl, "%s", _("missing \"")); return VSH_TK_ERROR; } @@ -10231,8 +10234,12 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) if (sz == 0) return VSH_TK_END; - *res = vshMalloc(ctl, sz + 1); - memcpy(*res, tkstr, sz); + if (!*res) { + *res = vshMalloc(ctl, sz + 1); + sz = 0; + p = q; + goto copy; + } *(*res + sz) = '\0'; *end = p;
On 10/12/2010 01:13 AM, Lai Jiangshan wrote:
In origin code, double quote is only allowed at the begin or end "complicated argument" --some_opt="complicated string" (we split this argument into 2 parts, option and data, the data is "complicated string").
This patch makes it allow double quote at any position of an argument: complicated" argument" complicated" "argument --"some opt=complicated string"
This patch is also needed for the following patches, the following patches will not split option argument into 2 parts, so we have to allow double quote at any position of an argument.
Signed-off-by: Lai Jiangshan<laijs@cn.fujitsu.com>
I had to add your name to AUTHORS to make syntax-check happy.
--- diff --git a/tools/virsh.c b/tools/virsh.c index 57ea618..7b6f2b6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10172,9 +10172,9 @@ static int vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) { int tk = VSH_TK_NONE; - int quote = FALSE; + bool double_quote = false; int sz = 0; - char *p = str; + char *p = str, *q;
Maybe it's just me, but I tend to declare multiple pointers on separate lines. But no big deal.
- } else if (quote&& *p == '"') { - quote = FALSE; + } + + if (*p == '"') {
'make syntax-check' would have caught this TAB.
+ double_quote = !double_quote; p++; - break; /* end of "..." token */ + continue; } + + if (*res) + (*res)[sz] = *p;
That's a lot of indirection, for every byte of the loop. Wouldn't it be better to have a local temporary, and only assign to *res at the end?
p++; sz++; } - if (quote) { + if (double_quote) { vshError(ctl, "%s", _("missing \"")); return VSH_TK_ERROR; } @@ -10231,8 +10234,12 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) if (sz == 0) return VSH_TK_END;
- *res = vshMalloc(ctl, sz + 1); - memcpy(*res, tkstr, sz); + if (!*res) { + *res = vshMalloc(ctl, sz + 1); + sz = 0; + p = q; + goto copy; + }
Hmm, a backwards jump, which means we parse every string twice - once to figure out how long it is, and again to strip quotes. Yes, that avoids over-allocating, but are we really that tight on memory? I find it quicker to just strdup() up front, then edit in-place on a single pass. Hmm, one thing you _don't_ recognize is: -"-"option as an option. In the shell, quotes are stripped before arguments are recognized as a particular token. I think that's pretty easy to support - delay VSH_TK_OPTION checking until after we've stripped quotes, but I'll show it as a separate patch. Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in this file; separate cleanup patch for that coming up soon. But, with those nits fixed, ACK. Here's what I'm squashing in to my local tree; I'll push it once I complete my review of your other 7 patches, as well as getting approval to my promised followup patches. diff --git i/AUTHORS w/AUTHORS index 09169f2..a8f96df 100644 --- i/AUTHORS +++ w/AUTHORS @@ -129,6 +129,7 @@ Patches have also been contributed by: diff --git i/tools/virsh.c w/tools/virsh.c index 0a28c99..4f70724 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -10124,16 +10124,18 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd) #define VSH_TK_DATA 2 #define VSH_TK_END 3 -static int +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) { int tk = VSH_TK_NONE; bool double_quote = false; int sz = 0; - char *p = str, *q; + char *p = str; + char *q = vshStrdup(ctl, str); char *tkstr = NULL; *end = NULL; + *res = q; while (p && *p && (*p == ' ' || *p == '\t')) p++; @@ -10145,9 +10147,6 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) return VSH_TK_END; } - q = p; - *res = NULL; -copy: while (*p) { /* end of token is blank space or ';' */ if (!double_quote && (*p == ' ' || *p == '\t' || *p == ';')) @@ -10170,34 +10169,23 @@ copy: tkstr = p; /* begin of token */ } - if (*p == '"') { + if (*p == '"') { double_quote = !double_quote; p++; continue; } - if (*res) - (*res)[sz] = *p; - p++; + *q++ = *p++; sz++; } if (double_quote) { vshError(ctl, "%s", _("missing \"")); return VSH_TK_ERROR; } - if (tkstr == NULL || *tkstr == '\0' || p == NULL) - return VSH_TK_END; - if (sz == 0) + if (tkstr == NULL || *tkstr == '\0' || sz == 0) return VSH_TK_END; - if (!*res) { - *res = vshMalloc(ctl, sz + 1); - sz = 0; - p = q; - goto copy; - } - *(*res + sz) = '\0'; - + *q = '\0'; *end = p; return tk; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* tools/virsh.c (malloc, calloc, realloc, strdup): Enforce that within this file, we use the safe vsh wrappers instead. (cmdNodeListDevices, cmdSnapshotCreate, main): Fix violations of this policy. ---
Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in this file; separate cleanup patch for that coming up soon.
The bulk of this patch is code motion. tools/virsh.c | 117 ++++++++++++++++++++++++++++++-------------------------- 1 files changed, 63 insertions(+), 54 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 4f70724..5abf218 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -316,6 +316,66 @@ static void *_vshRealloc(vshControl *ctl, void *ptr, size_t sz, const char *file static char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line); #define vshStrdup(_ctl, _s) _vshStrdup(_ctl, _s, __FILE__, __LINE__) +static void * +_vshMalloc(vshControl *ctl, size_t size, const char *filename, int line) +{ + void *x; + + if ((x = malloc(size))) + return x; + vshError(ctl, _("%s: %d: failed to allocate %d bytes"), + filename, line, (int) size); + exit(EXIT_FAILURE); +} + +static void * +_vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int line) +{ + void *x; + + if ((x = calloc(nmemb, size))) + return x; + vshError(ctl, _("%s: %d: failed to allocate %d bytes"), + filename, line, (int) (size*nmemb)); + exit(EXIT_FAILURE); +} + +static void * +_vshRealloc(vshControl *ctl, void *ptr, size_t size, const char *filename, int line) +{ + void *x; + + if ((x = realloc(ptr, size))) + return x; + VIR_FREE(ptr); + vshError(ctl, _("%s: %d: failed to allocate %d bytes"), + filename, line, (int) size); + exit(EXIT_FAILURE); +} + +static char * +_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line) +{ + char *x; + + if (s == NULL) + return(NULL); + if ((x = strdup(s))) + return x; + vshError(ctl, _("%s: %d: failed to allocate %lu bytes"), + filename, line, (unsigned long)strlen(s)); + exit(EXIT_FAILURE); +} + +/* Poison the raw allocating identifiers in favor of our vsh variants. */ +#undef malloc +#undef calloc +#undef realloc +#undef strdup +#define malloc use_vshMalloc_instead_of_malloc +#define calloc use_vshCalloc_instead_of_calloc +#define realloc use_vshRealloc_instead_of_realloc +#define strdup use_vshStrdup_instead_of_strdup static int idsorter(const void *a, const void *b) { const int *ia = (const int *)a; @@ -7253,7 +7313,7 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); if (dev && STRNEQ(devices[i], "computer")) { const char *parent = virNodeDeviceGetParent(dev); - parents[i] = parent ? strdup(parent) : NULL; + parents[i] = parent ? vshStrdup(ctl, parent) : NULL; } else { parents[i] = NULL; } @@ -8897,7 +8957,7 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) from = vshCommandOptString(cmd, "xmlfile", NULL); if (from == NULL) - buffer = strdup("<domainsnapshot/>"); + buffer = vshStrdup(ctl, "<domainsnapshot/>"); else { if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { /* we have to report the error here because during cleanup @@ -10442,57 +10502,6 @@ vshError(vshControl *ctl, const char *format, ...) fputc('\n', stderr); } -static void * -_vshMalloc(vshControl *ctl, size_t size, const char *filename, int line) -{ - void *x; - - if ((x = malloc(size))) - return x; - vshError(ctl, _("%s: %d: failed to allocate %d bytes"), - filename, line, (int) size); - exit(EXIT_FAILURE); -} - -static void * -_vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int line) -{ - void *x; - - if ((x = calloc(nmemb, size))) - return x; - vshError(ctl, _("%s: %d: failed to allocate %d bytes"), - filename, line, (int) (size*nmemb)); - exit(EXIT_FAILURE); -} - -static void * -_vshRealloc(vshControl *ctl, void *ptr, size_t size, const char *filename, int line) -{ - void *x; - - if ((x = realloc(ptr, size))) - return x; - VIR_FREE(ptr); - vshError(ctl, _("%s: %d: failed to allocate %d bytes"), - filename, line, (int) size); - exit(EXIT_FAILURE); -} - -static char * -_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line) -{ - char *x; - - if (s == NULL) - return(NULL); - if ((x = strdup(s))) - return x; - vshError(ctl, _("%s: %d: failed to allocate %lu bytes"), - filename, line, (unsigned long)strlen(s)); - exit(EXIT_FAILURE); -} - /* * Initialize connection. */ @@ -11074,7 +11083,7 @@ main(int argc, char **argv) ctl->log_fd = -1; /* Initialize log file descriptor */ if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) { - ctl->name = strdup(defaultConn); + ctl->name = vshStrdup(ctl, defaultConn); } if (!vshParseArgv(ctl, argc, argv)) { -- 1.7.2.3
On Tue, Oct 12, 2010 at 11:26:28AM -0600, Eric Blake wrote:
* tools/virsh.c (malloc, calloc, realloc, strdup): Enforce that within this file, we use the safe vsh wrappers instead. (cmdNodeListDevices, cmdSnapshotCreate, main): Fix violations of this policy. ---
Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in this file; separate cleanup patch for that coming up soon.
The bulk of this patch is code motion.
tools/virsh.c | 117 ++++++++++++++++++++++++++++++-------------------------- 1 files changed, 63 insertions(+), 54 deletions(-)
ACK, BTW I should do a followup patch, I had to change .x-sc_prohibit_strncpy to avoid virsh.c , but the real fix would have been to create a new safe vsh wrapper for strncpy instead, but this didn't fit well in the middle of the memory containment patches, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/12/2010 10:50 AM, Eric Blake wrote:
Hmm, one thing you _don't_ recognize is:
-"-"option
as an option. In the shell, quotes are stripped before arguments are recognized as a particular token. I think that's pretty easy to support - delay VSH_TK_OPTION checking until after we've stripped quotes, but I'll show it as a separate patch.
Nevermind. Patch 3 drops VSH_TK_OPTION altogether, at which point you've already delayed -- detection and nicely solved this problem without me needing any followup for that issue. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Lai Jiangshan