[Qemu-devel] [PATCH 0/2] Remove test-qga temporary file

[Qemu-devel] [PATCH 0/2] Remove test-qga temporary file

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Here are 2 small patches to remove the need for temporary file
creation in test-qga.

Marc-André Lureau (2):
  build-sys: define QEMU_SRC_PATH
  tests: use static qga config file

 scripts/create_config |  3 +++
 tests/test-qga-config |  8 ++++++++
 tests/test-qga.c      | 27 ++++-----------------------
 3 files changed, 15 insertions(+), 23 deletions(-)
 create mode 100644 tests/test-qga-config

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] build-sys: define QEMU_SRC_PATH

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Define QEMU_SRC_PATH in config-host.h, to ease accessing of tests data
files.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/create_config | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/create_config b/scripts/create_config
index 1dd6a35..2fbe126 100755
--- a/scripts/create_config
+++ b/scripts/create_config
@@ -116,6 +116,9 @@ case $line in
  DSOSUF=*)
     echo "#define HOST_DSOSUF \"${line#*=}\""
     ;;
+ SRC_PATH=*)
+     echo "#define QEMU_SRC_PATH \"${line#*=}\""
+     ;;
 esac
 
 done # read
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] build-sys: define QEMU_SRC_PATH

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2016-06-14 08:16:53)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Define QEMU_SRC_PATH in config-host.h, to ease accessing of tests data
> files.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I know this avoids the need to define environment variables for
individual test targets to pass on SRC_PATH, but the fact that
we didn't rely on this before made me a bit apprehensive about
suggesting it. qemu-iotests for instance relies on a symlink
back to SRC_PATH, and check-qapi-schema tests feed individual
schema files to a script via tests/Makefile.include. Both can
be made to check against a different/changing SRC_PATH, but
if we bake it into the code that's not possible.

I'm not sure if there's a valid use-case for needing to do
so, but it seems to be bad form to not have any mechanism
to change them without recompiling. Personally I think I'd
prefer the environment variable approach, even if it means
needing to add it per-target. I think if we wanted to get
fancy we could do this via a recipe that exports environment
variables added to a lazily-evaluated Makefile variable by
each target's dependencies, but it's probably not worth it
outside of a more general cleanup to how we handle
SRC_PATH/BUILD_DIR dependencies throughout tests/

If others are fine with the approach taken here though I
wouldn't hold things up.

> ---
>  scripts/create_config | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/create_config b/scripts/create_config
> index 1dd6a35..2fbe126 100755
> --- a/scripts/create_config
> +++ b/scripts/create_config
> @@ -116,6 +116,9 @@ case $line in
>   DSOSUF=*)
>      echo "#define HOST_DSOSUF \"${line#*=}\""
>      ;;
> + SRC_PATH=*)
> +     echo "#define QEMU_SRC_PATH \"${line#*=}\""
> +     ;;
>  esac
> 
>  done # read
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 1/2] build-sys: define QEMU_SRC_PATH

[Marc-André Lureau]

Hi

On Tue, Jun 14, 2016 at 11:59 PM, Michael Roth
<mdroth@linux.vnet.ibm.com> wrote:
> Quoting marcandre.lureau@redhat.com (2016-06-14 08:16:53)
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Define QEMU_SRC_PATH in config-host.h, to ease accessing of tests data
>> files.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I know this avoids the need to define environment variables for
> individual test targets to pass on SRC_PATH, but the fact that
> we didn't rely on this before made me a bit apprehensive about
> suggesting it. qemu-iotests for instance relies on a symlink
> back to SRC_PATH, and check-qapi-schema tests feed individual
> schema files to a script via tests/Makefile.include. Both can
> be made to check against a different/changing SRC_PATH, but
> if we bake it into the code that's not possible.

Right, so let's use a symlink too?

+if [ ! -e tests/test-qga-config ]; then
+    symlink "$source_path/tests/test-qga-config" tests/test-qga-config
+fi

>
> I'm not sure if there's a valid use-case for needing to do
> so, but it seems to be bad form to not have any mechanism
> to change them without recompiling. Personally I think I'd
> prefer the environment variable approach, even if it means
> needing to add it per-target. I think if we wanted to get
> fancy we could do this via a recipe that exports environment
> variables added to a lazily-evaluated Makefile variable by
> each target's dependencies, but it's probably not worth it
> outside of a more general cleanup to how we handle
> SRC_PATH/BUILD_DIR dependencies throughout tests/

I agree an environment variable would be nice (along with helper
functions to lookup test data files), but I think we should stick with
the common symlink way for now.

I'll resend the patch, thanks

-- 
Marc-André Lureau

[Qemu-devel] [PATCH 2/2] tests: use static qga config file

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Do not create a leaking temporary file, but use a static file instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/test-qga-config |  8 ++++++++
 tests/test-qga.c      | 27 ++++-----------------------
 2 files changed, 12 insertions(+), 23 deletions(-)
 create mode 100644 tests/test-qga-config

diff --git a/tests/test-qga-config b/tests/test-qga-config
new file mode 100644
index 0000000..4bb721a
--- /dev/null
+++ b/tests/test-qga-config
@@ -0,0 +1,8 @@
+[general]
+daemon=false
+method=virtio-serial
+path=/path/to/org.qemu.guest_agent.0
+pidfile=/var/foo/qemu-ga.pid
+statedir=/var/state
+verbose=true
+blacklist=guest-ping;guest-get-time
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 251b201..8686c23 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -691,28 +691,11 @@ static void test_qga_blacklist(gconstpointer data)
 static void test_qga_config(gconstpointer data)
 {
     GError *error = NULL;
-    char *cwd, *cmd, *out, *err, *str, **strv, *conf, **argv = NULL;
+    char *cwd, *cmd, *out, *err, *str, **strv, **argv = NULL;
     char *env[2];
-    int status, tmp;
+    int status;
     gsize n;
     GKeyFile *kf;
-    const char *qga_config =
-        "[general]\n"
-        "daemon=false\n"
-        "method=virtio-serial\n"
-        "path=/path/to/org.qemu.guest_agent.0\n"
-        "pidfile=/var/foo/qemu-ga.pid\n"
-        "statedir=/var/state\n"
-        "verbose=true\n"
-        "blacklist=guest-ping;guest-get-time\n";
-
-    tmp = g_file_open_tmp(NULL, &conf, &error);
-    g_assert_no_error(error);
-    g_assert_cmpint(tmp, >=, 0);
-    g_assert_cmpstr(conf, !=, "");
-
-    g_file_set_contents(conf, qga_config, -1, &error);
-    g_assert_no_error(error);
 
     cwd = g_get_current_dir();
     cmd = g_strdup_printf("%s%cqemu-ga -D",
@@ -720,7 +703,8 @@ static void test_qga_config(gconstpointer data)
     g_shell_parse_argv(cmd, NULL, &argv, &error);
     g_assert_no_error(error);
 
-    env[0] = g_strdup_printf("QGA_CONF=%s", conf);
+    env[0] = g_strdup_printf("QGA_CONF=%s%ctests%ctest-qga-config",
+                             QEMU_SRC_PATH, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
     g_spawn_sync(NULL, argv, env, 0,
                  NULL, NULL, &out, &err, &status, &error);
@@ -775,11 +759,8 @@ static void test_qga_config(gconstpointer data)
 
     g_free(out);
     g_free(err);
-    g_free(conf);
     g_free(env[0]);
     g_key_file_free(kf);
-
-    close(tmp);
 }
 
 static void test_qga_fsfreeze_status(gconstpointer fix)
-- 
2.7.4