[Qemu-devel] [PATCH for-v2.7 v2 0/2] test-logging: don't hard-code paths in /tmp

[Qemu-devel] [PATCH for-v2.7 v2 0/2] test-logging: don't hard-code paths in /tmp

[Sascha Silbe]

This version should be good enough for inclusion in 2.7. I kept the
temporary directory removal function local to test-logging for now,
only cleaning up a single directory level. We can still factor it out
and make it more generic in the 2.8 cycle. For 2.7 I'd rather stick
with a minimal approach as it's very late in the cycle.

Tested successfully with the centos6 docker image. Apart from a hang
(tests/test-qga) and a race condition (tests/acpi-test-disk.raw
missing) that both happen even without my patches, it also works well
on Ubuntu 14.04.

Feel free to perform any additional fixup required to land this in
rc4; I might not be around again until Tuesday.

Sascha Silbe (2):
  glib: add compatibility implementation for g_dir_make_tmp()
  test-logging: don't hard-code paths in /tmp

 include/glib-compat.h | 21 +++++++++++++++++++++
 tests/test-logging.c  | 48 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 7 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/2] glib: add compatibility implementation for g_dir_make_tmp()

[Sascha Silbe]

We're going to make use of g_dir_make_tmp() in test-logging. Provide a
compatibility implementation of it for glib < 2.30.

May behave differently in some edge cases (e.g. pattern only at the
end of the template, the file name is not part of the error message),
but good enough in practice.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
v1→v2:
  - new patch

 include/glib-compat.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 01aa7b3..de64ac2 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -48,6 +48,27 @@ static inline gint64 qemu_g_get_monotonic_time(void)
 gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
 #endif
 
+#if !GLIB_CHECK_VERSION(2, 30, 0)
+/* Not a 100% compatible implementation, but good enough for most
+ * cases. Placeholders are only supported at the end of the
+ * template. */
+static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
+{
+    gchar const *template = tmpl ? tmpl : ".XXXXXX";
+    gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
+
+    if (mkdtemp(path) != NULL) {
+        return path;
+    }
+    /* Error occurred, clean up. */
+    g_set_error(error, G_FILE_ERROR, g_file_error_from_errno(errno),
+                "mkdtemp() failed");
+    g_free(path);
+    return NULL;
+}
+#define g_dir_make_tmp(tmpl, error) qemu_g_dir_make_tmp(tmpl, error)
+#endif /* glib 2.30 */
+
 #if !GLIB_CHECK_VERSION(2, 31, 0)
 /* before glib-2.31, GMutex and GCond was dynamic-only (there was a separate
  * GStaticMutex, but it didn't work with condition variables).
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 1/2] glib: add compatibility implementation for g_dir_make_tmp()

[Peter Maydell]

On 18 August 2016 at 19:46, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> We're going to make use of g_dir_make_tmp() in test-logging. Provide a
> compatibility implementation of it for glib < 2.30.
>
> May behave differently in some edge cases (e.g. pattern only at the
> end of the template, the file name is not part of the error message),
> but good enough in practice.
>
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> ---
> v1→v2:
>   - new patch
>
>  include/glib-compat.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 01aa7b3..de64ac2 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -48,6 +48,27 @@ static inline gint64 qemu_g_get_monotonic_time(void)
>  gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
>  #endif
>
> +#if !GLIB_CHECK_VERSION(2, 30, 0)
> +/* Not a 100% compatible implementation, but good enough for most
> + * cases. Placeholders are only supported at the end of the
> + * template. */
> +static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
> +{
> +    gchar const *template = tmpl ? tmpl : ".XXXXXX";

This doesn't build:
In file included from /Users/pm215/src/qemu-for-merges/disas/arm-a64.cc:21:
In file included from /Users/pm215/src/qemu-for-merges/include/qemu/osdep.h:107:
/Users/pm215/src/qemu-for-merges/include/glib-compat.h:57:18: error:
expected unqualified-id
    gchar const *template = tmpl ? tmpl : ".XXXXXX";
                 ^
/Users/pm215/src/qemu-for-merges/include/glib-compat.h:58:53: error:
expected expression
    gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
                                                    ^

because this header is included from some C++ files, where
"template" is a reserved word. I've fixed this by squashing
in the following change:

--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -54,8 +54,7 @@ gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
  * template. */
 static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
 {
-    gchar const *template = tmpl ? tmpl : ".XXXXXX";
-    gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
+    gchar *path = g_build_filename(g_get_tmp_dir(), tmpl ?: ".XXXXXX", NULL);

     if (mkdtemp(path) != NULL) {
         return path;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/2] glib: add compatibility implementation for g_dir_make_tmp()

[Sascha Silbe]

Dear Peter,

Peter Maydell <peter.maydell@linaro.org> writes:

[...]
>> +#if !GLIB_CHECK_VERSION(2, 30, 0)
>> +/* Not a 100% compatible implementation, but good enough for most
>> + * cases. Placeholders are only supported at the end of the
>> + * template. */
>> +static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
>> +{
>> +    gchar const *template = tmpl ? tmpl : ".XXXXXX";
>
> This doesn't build:
> In file included from /Users/pm215/src/qemu-for-merges/disas/arm-a64.cc:21:
> In file included from /Users/pm215/src/qemu-for-merges/include/qemu/osdep.h:107:
> /Users/pm215/src/qemu-for-merges/include/glib-compat.h:57:18: error:
> expected unqualified-id
>     gchar const *template = tmpl ? tmpl : ".XXXXXX";
>                  ^
> /Users/pm215/src/qemu-for-merges/include/glib-compat.h:58:53: error:
> expected expression
>     gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
>                                                     ^
>
> because this header is included from some C++ files, where
> "template" is a reserved word. I've fixed this by squashing
> in the following change:

Curious, didn't hit this on my laptop (Ubuntu 14.04, x86_64) and neither
in the centos6 docker image. At least on my laptop arm-a64.cc gets
compiled and -Werror is enabled, too. Does this happen with a particular
tool chain only? clang perchance?

FWIW, I would have expected the C language linkage specification
(i.e. extern "C" {}) in disas/arm-a64.cc to take care of this kind of
issue. However after scanning through C++11 it seems the linkage
specification only affects external linkage names. Plus is doesn't seem
keywords are valid identifiers even for external C language linkage
names; there's only an exception for attributes, not for non-C++
language linkage.


[From second mail]
> Applied to master (with minor fixup to patch 1); thanks.

Thanks!

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

[Qemu-devel] [PATCH v2 2/2] test-logging: don't hard-code paths in /tmp

[Sascha Silbe]

Since f6880b7f [qemu-log: support simple pid substitution for logs],
test-logging creates files with hard-coded names in /tmp. In the best
case, this prevents multiple developers from running "make check" on
the same machine. In the worst case, it allows for symlink attacks,
enabling an attacker to overwrite files that are writable to the
developer running "make check".

Instead of hard-coding the paths, create a temporary directory using
g_dir_make_tmp() and clean it up afterwards.

Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
v1→v2:
  - Factor out g_build_filename() + qemu_set_log_filename() + g_free()
    into helper set_log_path_tmp().
  - Replace rmtree() spawning "rm -rf" with rmdir_full() using glib
    directory handling (non-recursive). Private to test-logging for
    now.

 tests/test-logging.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index cdf13c6..a12585f 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include <glib/gstdio.h>
 
 #include "qemu-common.h"
 #include "qapi/error.h"
@@ -86,24 +87,57 @@ static void test_parse_range(void)
     error_free_or_abort(&err);
 }
 
-static void test_parse_path(void)
+static void set_log_path_tmp(char const *dir, char const *tpl, Error **errp)
 {
+    gchar *file_path = g_build_filename(dir, tpl, NULL);
+
+    qemu_set_log_filename(file_path, errp);
+    g_free(file_path);
+}
+
+static void test_parse_path(gconstpointer data)
+{
+    gchar const *tmp_path = data;
     Error *err = NULL;
 
-    qemu_set_log_filename("/tmp/qemu.log", &error_abort);
-    qemu_set_log_filename("/tmp/qemu-%d.log", &error_abort);
-    qemu_set_log_filename("/tmp/qemu.log.%d", &error_abort);
+    set_log_path_tmp(tmp_path, "qemu.log", &error_abort);
+    set_log_path_tmp(tmp_path, "qemu-%d.log", &error_abort);
+    set_log_path_tmp(tmp_path, "qemu.log.%d", &error_abort);
 
-    qemu_set_log_filename("/tmp/qemu-%d%d.log", &err);
+    set_log_path_tmp(tmp_path, "qemu-%d%d.log", &err);
     error_free_or_abort(&err);
 }
 
+/* Remove a directory and all its entries (non-recursive). */
+static void rmdir_full(gchar const *root)
+{
+    GDir *root_gdir = g_dir_open(root, 0, NULL);
+    gchar const *entry_name;
+
+    g_assert_nonnull(root_gdir);
+    while ((entry_name = g_dir_read_name(root_gdir)) != NULL) {
+        gchar *entry_path = g_build_filename(root, entry_name, NULL);
+        g_assert(g_remove(entry_path) == 0);
+        g_free(entry_path);
+    }
+    g_dir_close(root_gdir);
+    g_assert(g_rmdir(root) == 0);
+}
+
 int main(int argc, char **argv)
 {
+    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
+    int rc;
+
     g_test_init(&argc, &argv, NULL);
+    g_assert_nonnull(tmp_path);
 
     g_test_add_func("/logging/parse_range", test_parse_range);
-    g_test_add_func("/logging/parse_path", test_parse_path);
+    g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path);
 
-    return g_test_run();
+    rc = g_test_run();
+
+    rmdir_full(tmp_path);
+    g_free(tmp_path);
+    return rc;
 }
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-v2.7 v2 0/2] test-logging: don't hard-code paths in /tmp

[Peter Maydell]

On 18 August 2016 at 19:46, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> This version should be good enough for inclusion in 2.7. I kept the
> temporary directory removal function local to test-logging for now,
> only cleaning up a single directory level. We can still factor it out
> and make it more generic in the 2.8 cycle. For 2.7 I'd rather stick
> with a minimal approach as it's very late in the cycle.
>
> Tested successfully with the centos6 docker image. Apart from a hang
> (tests/test-qga) and a race condition (tests/acpi-test-disk.raw
> missing) that both happen even without my patches, it also works well
> on Ubuntu 14.04.
>
> Feel free to perform any additional fixup required to land this in
> rc4; I might not be around again until Tuesday.
>
> Sascha Silbe (2):
>   glib: add compatibility implementation for g_dir_make_tmp()
>   test-logging: don't hard-code paths in /tmp

Applied to master (with minor fixup to patch 1); thanks.

-- PMM