3:13 p.m.
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(a)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;
12:50 a.m.
New subject: [libvirt] [PATCH 1/8] virsh: better support double quote
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:26 a.m.
New subject: [libvirt] [PATCH] virsh: poison raw allocation routines
* 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
8:14 p.m.
New subject: [libvirt] [PATCH] virsh: poison raw allocation routines
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
2:02 a.m.
New subject: [libvirt] [PATCH 1/8] virsh: better support double quote
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5131
days inactive
5132
days old
4 comments
3 participants
participants (3)
-
Daniel Veillard -
Eric Blake -
Lai Jiangshan