On 02/25/2012 05:48 PM, Josh Durgin wrote:
> QMP commands don't need to be escaped since converting them to json
> also escapes special characters. When a QMP command fails, however,
> libvirt falls back to HMP commands. These fallback functions
> (qemuMonitorText*) do their own escaping, and pass the result directly
> to qemuMonitorHMPCommandWithFd. If the monitor is in json mode, these
> pre-escaped commands will be escaped again when converted to json,
> which can result in the wrong arguments being sent.
>
> For example, a filename test\file would be sent in json as
> test\\file.
>
> This prevented attaching an image file with a " or \ in its name in
> qemu 1.0.50, and also broke rbd attachment (which uses backslashes to
> escape some internal arguments.)
>
> Reported-by: Masuko Tomoya <tomoya.masuko@xxxxxxxxx>
> Signed-off-by: Josh Durgin <josh.durgin@xxxxxxxxxxxxx>
> ---
>
> Changes since v1:
> * fix leak of json_cmd
> * change comments to /* */ instead of //
>
> .gitignore | 1 +
> src/qemu/qemu_monitor.c | 67 ++++++++++++++++++++++++++--
> src/qemu/qemu_monitor.h | 1 +
> tests/Makefile.am | 12 ++++-
> tests/qemumonitortest.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 188 insertions(+), 7 deletions(-)
> create mode 100644 tests/qemumonitortest.c
>
> diff --git a/.gitignore b/.gitignore
> index b7561dc..264a419 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -128,6 +128,7 @@
> /tests/openvzutilstest
> /tests/qemuargv2xmltest
> /tests/qemuhelptest
> +/tests/qemumonitortest
Nice - adding a new test alongside a bug fix is always appreciated.
> +++ b/tests/qemumonitortest.c
> @@ -0,0 +1,114 @@
> +#include <config.h>
Adding new files without copyright is bad practice. But you are copying
existing practice in that directory; which means I'm assuming that you
are okay if we later make a global pass on that directory and add a
header on all files that need one, whether that header is LGPLv2+ (like
most of the rest of the project), GPLv3+ (since tests aren't installed
programs), or anything else. I'll go ahead and commit this as is, but
please speak up if my assumptions about globally adding an appropriate
copyright header to all tests in the future would give you grief.
> +struct testEscapeString
> +{
> + const char* unescaped;
> + const char* escaped;
Associate the * with the variable, not the type.
> +static int testEscapeArg(const void *data ATTRIBUTE_UNUSED)
> +{
> + int i;
> + char *escaped = NULL;
> + for (i = 0; i < ARRAY_CARDINALITY(escapeStrings); ++i) {
> + escaped = qemuMonitorEscapeArg(escapeStrings[i].unescaped);
> + if (!escaped) {
> + if (virTestGetDebug() > 0) {
> + fprintf(stderr, "\nUnescaped string [%s]\n", escapeStrings[i].unescaped);
> + fprintf(stderr, "Expect result [%s]\n", escapeStrings[i].escaped);
Long lines.
> + fprintf(stderr, "Actual result [%s]\n", escaped);
Missing the NULLSTR() wrapper around escaped. Or, since we _know_
escaped is NULL, we can inline the effects of the NULLSTR() wrapper in
the first place.
> +static int testUnescapeArg(const void *data ATTRIBUTE_UNUSED)
> +{
> + int i;
> + char *unescaped = NULL;
> + for (i = 0; i < ARRAY_CARDINALITY(escapeStrings); ++i) {
> + unescaped = qemuMonitorUnescapeArg(escapeStrings[i].escaped);
> + if (!unescaped) {
> + if (virTestGetDebug() > 0) {
> + fprintf(stderr, "\nEscaped string [%s]\n", escapeStrings[i].escaped);
> + fprintf(stderr, "Expect result [%s]\n", escapeStrings[i].unescaped);
> + fprintf(stderr, "Actual result [%s]\n", unescaped);
Same problems in reverse :)
> +static int
> +mymain(void)
> +{
> + int result = 0;
> +
> +#define DO_TEST(_name) \
If you install 'cppi', then 'make syntax-check' complains about this line.
ACK with the nits fixed, so I squashed this and pushed.
diff --git i/tests/qemumonitortest.c w/tests/qemumonitortest.c
index bf90502..cf460ad 100644
--- i/tests/qemumonitortest.c
+++ w/tests/qemumonitortest.c
@@ -15,8 +15,8 @@
struct testEscapeString
{
- const char* unescaped;
- const char* escaped;
+ const char *unescaped;
+ const char *escaped;
};
static struct testEscapeString escapeStrings[] = {
@@ -40,9 +40,11 @@ static int testEscapeArg(const void *data
ATTRIBUTE_UNUSED)
escaped = qemuMonitorEscapeArg(escapeStrings[i].unescaped);
if (!escaped) {
if (virTestGetDebug() > 0) {
- fprintf(stderr, "\nUnescaped string [%s]\n",
escapeStrings[i].unescaped);
- fprintf(stderr, "Expect result [%s]\n",
escapeStrings[i].escaped);
- fprintf(stderr, "Actual result [%s]\n", escaped);
+ fprintf(stderr, "\nUnescaped string [%s]\n",
+ escapeStrings[i].unescaped);
+ fprintf(stderr, "Expect result [%s]\n",
+ escapeStrings[i].escaped);
+ fprintf(stderr, "Actual result [(null)]\n");
}
return -1;
}
@@ -65,9 +67,11 @@ static int testUnescapeArg(const void *data
ATTRIBUTE_UNUSED)
unescaped = qemuMonitorUnescapeArg(escapeStrings[i].escaped);
if (!unescaped) {
if (virTestGetDebug() > 0) {
- fprintf(stderr, "\nEscaped string [%s]\n",
escapeStrings[i].escaped);
- fprintf(stderr, "Expect result [%s]\n",
escapeStrings[i].unescaped);
- fprintf(stderr, "Actual result [%s]\n", unescaped);
+ fprintf(stderr, "\nEscaped string [%s]\n",
+ escapeStrings[i].escaped);
+ fprintf(stderr, "Expect result [%s]\n",
+ escapeStrings[i].unescaped);
+ fprintf(stderr, "Actual result [(null)]\n");
}
return -1;
}
@@ -87,7 +91,7 @@ mymain(void)
{
int result = 0;
-#define DO_TEST(_name) \
+# define DO_TEST(_name) \
do {
\
if (virtTestRun("qemu monitor "#_name, 1, test##_name,
\
NULL) < 0) {
\
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list