All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 00/15] Misc cleanups
@ 2022-05-13 18:08 marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
                   ` (15 more replies)
  0 siblings, 16 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Hi,

v3:
- changed error_report_err() back to g_critical()
- added "qga: make build_fs_mount_list() return a bool"
- replaced g_clear_pointer() usage by open-coded version
- dropped needless g_autoptr(GError) in tests
- rebased, (dropped "include: adjust header guards after renaming")
- some commit message rewording
- added r-b tags

v2:
- drop "compiler.h: add QEMU_{BEGIN,END}_IGNORE_INITIALIZER_OVERRIDES",
  "qobject/json-lexer: disable -Winitializer-overrides warnings" &
  "qapi/error: add g_autoptr(Error) support" and adjust related code.
- add "test/qga: use g_auto wherever sensible"
- add r-b tags

Marc-André Lureau (15):
  include: move qemu_*_exec_dir() to cutils
  util/win32: simplify qemu_get_local_state_dir()
  tests: make libqmp buildable for win32
  qga: flatten safe_open_or_create()
  osdep: export qemu_open_cloexec()
  qga: use qemu_open_cloexec() for safe_open_or_create()
  qga: throw an Error in ga_channel_open()
  qga: replace qemu_open_old() with qemu_open_cloexec()
  qga: make build_fs_mount_list() return a bool
  test/qga: use G_TEST_DIR to locate os-release test file
  qga/wixl: prefer variables over environment
  qga/wixl: require Mingw_bin
  qga/wixl: simplify some pre-processing
  qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
  test/qga: use g_auto wherever sensible

 configure                            |   9 +-
 include/qemu/cutils.h                |   7 ++
 include/qemu/osdep.h                 |   9 +-
 meson.build                          |   5 +-
 qemu-io.c                            |   1 +
 qga/channel-posix.c                  |  55 +++++----
 qga/commands-posix.c                 | 164 +++++++++++++--------------
 qga/installer/qemu-ga.wxs            |  83 +++++---------
 qga/meson.build                      |  11 +-
 storage-daemon/qemu-storage-daemon.c |   1 +
 tests/qtest/fuzz/fuzz.c              |   1 +
 tests/qtest/libqmp.c                 |  34 +++++-
 tests/qtest/libqmp.h                 |   2 +
 tests/unit/test-qga.c                | 130 ++++++++-------------
 util/cutils.c                        | 108 ++++++++++++++++++
 util/osdep.c                         |  10 +-
 util/oslib-posix.c                   |  81 -------------
 util/oslib-win32.c                   |  53 +--------
 18 files changed, 358 insertions(+), 406 deletions(-)

-- 
2.36.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-23 12:23   ` Markus Armbruster
  2022-05-13 18:08 ` [PATCH v3 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
                   ` (14 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

The function is required by get_relocated_path() (already in cutils),
and used by qemu-ga and may be generally useful.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/cutils.h                |   7 ++
 include/qemu/osdep.h                 |   8 --
 qemu-io.c                            |   1 +
 storage-daemon/qemu-storage-daemon.c |   1 +
 tests/qtest/fuzz/fuzz.c              |   1 +
 util/cutils.c                        | 108 +++++++++++++++++++++++++++
 util/oslib-posix.c                   |  81 --------------------
 util/oslib-win32.c                   |  36 ---------
 8 files changed, 118 insertions(+), 125 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 5c6572d444..40e10e19a7 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -193,6 +193,13 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
  */
 int qemu_pstrcmp0(const char **str1, const char **str2);
 
+/* Find program directory, and save it for later usage with
+ * qemu_get_exec_dir().
+ * Try OS specific API first, if not working, parse from argv0. */
+void qemu_init_exec_dir(const char *argv0);
+
+/* Get the saved exec dir.  */
+const char *qemu_get_exec_dir(void);
 
 /**
  * get_relocated_path:
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1c1e7eca98..67cc465416 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -557,14 +557,6 @@ void qemu_set_cloexec(int fd);
  */
 char *qemu_get_local_state_dir(void);
 
-/* Find program directory, and save it for later usage with
- * qemu_get_exec_dir().
- * Try OS specific API first, if not working, parse from argv0. */
-void qemu_init_exec_dir(const char *argv0);
-
-/* Get the saved exec dir.  */
-const char *qemu_get_exec_dir(void);
-
 /**
  * qemu_getauxval:
  * @type: the auxiliary vector key to lookup
diff --git a/qemu-io.c b/qemu-io.c
index d70d3dd4fd..2bd7bfb650 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -16,6 +16,7 @@
 #endif
 
 #include "qemu/help-texts.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qemu-io.h"
 #include "qemu/error-report.h"
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 9b8b17f52e..c104817cdd 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -44,6 +44,7 @@
 
 #include "qemu/help-texts.h"
 #include "qemu-version.h"
+#include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index a7a5e14fa3..0ad4ba9e94 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -15,6 +15,7 @@
 
 #include <wordexp.h>
 
+#include "qemu/cutils.h"
 #include "qemu/datadir.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/qtest.h"
diff --git a/util/cutils.c b/util/cutils.c
index b2777210e7..6cc7cc8cde 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -931,6 +931,114 @@ static inline const char *next_component(const char *dir, int *p_len)
     return dir;
 }
 
+static const char *exec_dir;
+
+void qemu_init_exec_dir(const char *argv0)
+{
+#ifdef G_OS_WIN32
+    char *p;
+    char buf[MAX_PATH];
+    DWORD len;
+
+    if (exec_dir) {
+        return;
+    }
+
+    len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
+    if (len == 0) {
+        return;
+    }
+
+    buf[len] = 0;
+    p = buf + len - 1;
+    while (p != buf && *p != '\\') {
+        p--;
+    }
+    *p = 0;
+    if (access(buf, R_OK) == 0) {
+        exec_dir = g_strdup(buf);
+    } else {
+        exec_dir = CONFIG_BINDIR;
+    }
+#else
+    char *p = NULL;
+    char buf[PATH_MAX];
+
+    if (exec_dir) {
+        return;
+    }
+
+#if defined(__linux__)
+    {
+        int len;
+        len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
+        if (len > 0) {
+            buf[len] = 0;
+            p = buf;
+        }
+    }
+#elif defined(__FreeBSD__) \
+      || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
+    {
+#if defined(__FreeBSD__)
+        static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
+#else
+        static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};
+#endif
+        size_t len = sizeof(buf) - 1;
+
+        *buf = '\0';
+        if (!sysctl(mib, ARRAY_SIZE(mib), buf, &len, NULL, 0) &&
+            *buf) {
+            buf[sizeof(buf) - 1] = '\0';
+            p = buf;
+        }
+    }
+#elif defined(__APPLE__)
+    {
+        char fpath[PATH_MAX];
+        uint32_t len = sizeof(fpath);
+        if (_NSGetExecutablePath(fpath, &len) == 0) {
+            p = realpath(fpath, buf);
+            if (!p) {
+                return;
+            }
+        }
+    }
+#elif defined(__HAIKU__)
+    {
+        image_info ii;
+        int32_t c = 0;
+
+        *buf = '\0';
+        while (get_next_image_info(0, &c, &ii) == B_OK) {
+            if (ii.type == B_APP_IMAGE) {
+                strncpy(buf, ii.name, sizeof(buf));
+                buf[sizeof(buf) - 1] = 0;
+                p = buf;
+                break;
+            }
+        }
+    }
+#endif
+    /* If we don't have any way of figuring out the actual executable
+       location then try argv[0].  */
+    if (!p && argv0) {
+        p = realpath(argv0, buf);
+    }
+    if (p) {
+        exec_dir = g_path_get_dirname(p);
+    } else {
+        exec_dir = CONFIG_BINDIR;
+    }
+#endif
+}
+
+const char *qemu_get_exec_dir(void)
+{
+    return exec_dir;
+}
+
 char *get_relocated_path(const char *dir)
 {
     size_t prefix_len = strlen(CONFIG_PREFIX);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 477990f39b..7ba4472760 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -283,87 +283,6 @@ void qemu_set_tty_echo(int fd, bool echo)
     tcsetattr(fd, TCSANOW, &tty);
 }
 
-static const char *exec_dir;
-
-void qemu_init_exec_dir(const char *argv0)
-{
-    char *p = NULL;
-    char buf[PATH_MAX];
-
-    if (exec_dir) {
-        return;
-    }
-
-#if defined(__linux__)
-    {
-        int len;
-        len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
-        if (len > 0) {
-            buf[len] = 0;
-            p = buf;
-        }
-    }
-#elif defined(__FreeBSD__) \
-      || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
-    {
-#if defined(__FreeBSD__)
-        static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
-#else
-        static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};
-#endif
-        size_t len = sizeof(buf) - 1;
-
-        *buf = '\0';
-        if (!sysctl(mib, ARRAY_SIZE(mib), buf, &len, NULL, 0) &&
-            *buf) {
-            buf[sizeof(buf) - 1] = '\0';
-            p = buf;
-        }
-    }
-#elif defined(__APPLE__)
-    {
-        char fpath[PATH_MAX];
-        uint32_t len = sizeof(fpath);
-        if (_NSGetExecutablePath(fpath, &len) == 0) {
-            p = realpath(fpath, buf);
-            if (!p) {
-                return;
-            }
-        }
-    }
-#elif defined(__HAIKU__)
-    {
-        image_info ii;
-        int32_t c = 0;
-
-        *buf = '\0';
-        while (get_next_image_info(0, &c, &ii) == B_OK) {
-            if (ii.type == B_APP_IMAGE) {
-                strncpy(buf, ii.name, sizeof(buf));
-                buf[sizeof(buf) - 1] = 0;
-                p = buf;
-                break;
-            }
-        }
-    }
-#endif
-    /* If we don't have any way of figuring out the actual executable
-       location then try argv[0].  */
-    if (!p && argv0) {
-        p = realpath(argv0, buf);
-    }
-    if (p) {
-        exec_dir = g_path_get_dirname(p);
-    } else {
-        exec_dir = CONFIG_BINDIR;
-    }
-}
-
-const char *qemu_get_exec_dir(void)
-{
-    return exec_dir;
-}
-
 #ifdef CONFIG_LINUX
 static void sigbus_handler(int signal, siginfo_t *siginfo, void *ctx)
 #else /* CONFIG_LINUX */
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index dafef4f157..6c818749d2 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -269,42 +269,6 @@ void qemu_set_tty_echo(int fd, bool echo)
     }
 }
 
-static const char *exec_dir;
-
-void qemu_init_exec_dir(const char *argv0)
-{
-
-    char *p;
-    char buf[MAX_PATH];
-    DWORD len;
-
-    if (exec_dir) {
-        return;
-    }
-
-    len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
-    if (len == 0) {
-        return;
-    }
-
-    buf[len] = 0;
-    p = buf + len - 1;
-    while (p != buf && *p != '\\') {
-        p--;
-    }
-    *p = 0;
-    if (access(buf, R_OK) == 0) {
-        exec_dir = g_strdup(buf);
-    } else {
-        exec_dir = CONFIG_BINDIR;
-    }
-}
-
-const char *qemu_get_exec_dir(void)
-{
-    return exec_dir;
-}
-
 int getpagesize(void)
 {
     SYSTEM_INFO system_info;
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 02/15] util/win32: simplify qemu_get_local_state_dir()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 03/15] tests: make libqmp buildable for win32 marcandre.lureau
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

SHGetFolderPath() is a deprecated API:
https://docs.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetfolderpatha

It is a wrapper for SHGetKnownFolderPath() and CSIDL_COMMON_PATH is
mapped to FOLDERID_ProgramData:
https://docs.microsoft.com/en-us/windows/win32/shell/csidl

g_get_system_data_dirs() is a suitable replacement, as it will have
FOLDERID_ProgramData in the returned list. However, it follows the XDG
Base Directory Specification, if `XDG_DATA_DIRS` is defined, it will be
returned instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
---
 util/oslib-win32.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 6c818749d2..5723d3eb4c 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -40,9 +40,6 @@
 #include "qemu/error-report.h"
 #include <malloc.h>
 
-/* this must come after including "trace.h" */
-#include <shlobj.h>
-
 static int get_allocation_granularity(void)
 {
     SYSTEM_INFO system_info;
@@ -237,17 +234,11 @@ int qemu_get_thread_id(void)
 char *
 qemu_get_local_state_dir(void)
 {
-    HRESULT result;
-    char base_path[MAX_PATH+1] = "";
+    const char * const *data_dirs = g_get_system_data_dirs();
 
-    result = SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL,
-                             /* SHGFP_TYPE_CURRENT */ 0, base_path);
-    if (result != S_OK) {
-        /* misconfigured environment */
-        g_critical("CSIDL_COMMON_APPDATA unavailable: %ld", (long)result);
-        abort();
-    }
-    return g_strdup(base_path);
+    g_assert(data_dirs && data_dirs[0]);
+
+    return g_strdup(data_dirs[0]);
 }
 
 void qemu_set_tty_echo(int fd, bool echo)
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 03/15] tests: make libqmp buildable for win32
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 04/15] qga: flatten safe_open_or_create() marcandre.lureau
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/qtest/libqmp.c | 34 +++++++++++++++++++++++++++++-----
 tests/qtest/libqmp.h |  2 ++
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
index 0358b8313d..df39d250fe 100644
--- a/tests/qtest/libqmp.c
+++ b/tests/qtest/libqmp.c
@@ -18,6 +18,11 @@
 
 #include "libqmp.h"
 
+#ifndef G_OS_WIN32
+#include <sys/socket.h>
+#endif
+
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qjson.h"
@@ -87,6 +92,7 @@ QDict *qmp_fd_receive(int fd)
     return qmp.response;
 }
 
+#ifndef G_OS_WIN32
 /* Sends a message and file descriptors to the socket.
  * It's needed for qmp-commands like getfd/add-fd */
 static void socket_send_fds(int socket_fd, int *fds, size_t fds_num,
@@ -120,17 +126,23 @@ static void socket_send_fds(int socket_fd, int *fds, size_t fds_num,
     } while (ret < 0 && errno == EINTR);
     g_assert_cmpint(ret, >, 0);
 }
+#endif
 
 /**
  * Allow users to send a message without waiting for the reply,
  * in the case that they choose to discard all replies up until
  * a particular EVENT is received.
  */
-void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
-                      const char *fmt, va_list ap)
+static void
+_qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
+                  const char *fmt, va_list ap)
 {
     QObject *qobj;
 
+#ifdef G_OS_WIN32
+    assert(fds_num == 0);
+#endif
+
     /* Going through qobject ensures we escape strings properly */
     qobj = qobject_from_vjsonf_nofail(fmt, ap);
 
@@ -148,10 +160,14 @@ void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
         if (log) {
             fprintf(stderr, "%s", str->str);
         }
+
+#ifndef G_OS_WIN32
         /* Send QMP request */
         if (fds && fds_num > 0) {
             socket_send_fds(fd, fds, fds_num, str->str, str->len);
-        } else {
+        } else
+#endif
+        {
             socket_send(fd, str->str, str->len);
         }
 
@@ -160,15 +176,23 @@ void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
     }
 }
 
+#ifndef G_OS_WIN32
+void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
+                      const char *fmt, va_list ap)
+{
+    _qmp_fd_vsend_fds(fd, fds, fds_num, fmt, ap);
+}
+#endif
+
 void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
 {
-    qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
+    _qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
 }
 
 
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
 {
-    qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
+    _qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
 
     return qmp_fd_receive(fd);
 }
diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
index 5cb7eeaa18..9e9768f559 100644
--- a/tests/qtest/libqmp.h
+++ b/tests/qtest/libqmp.h
@@ -21,8 +21,10 @@
 #include "qapi/qmp/qdict.h"
 
 QDict *qmp_fd_receive(int fd);
+#ifndef G_OS_WIN32
 void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
                       const char *fmt, va_list ap) G_GNUC_PRINTF(4, 0);
+#endif
 void qmp_fd_vsend(int fd, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0);
 void qmp_fd_send(int fd, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 void qmp_fd_send_raw(int fd, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 04/15] qga: flatten safe_open_or_create()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (2 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 03/15] tests: make libqmp buildable for win32 marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-16  7:19   ` Markus Armbruster
  2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
                   ` (11 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

There is a bit too much nesting in the function, this can be simplified
a bit to improve readability.

This also helps with the following error handling changes.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c | 122 ++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 69f209af87..15eb7cb77d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -339,73 +339,75 @@ find_open_flag(const char *mode_str, Error **errp)
 static FILE *
 safe_open_or_create(const char *path, const char *mode, Error **errp)
 {
-    Error *local_err = NULL;
     int oflag;
+    int fd = -1;
+    FILE *f = NULL;
+
+    oflag = find_open_flag(mode, errp);
+    if (oflag < 0) {
+        goto end;
+    }
+
+    /* If the caller wants / allows creation of a new file, we implement it
+     * with a two step process: open() + (open() / fchmod()).
+     *
+     * First we insist on creating the file exclusively as a new file. If
+     * that succeeds, we're free to set any file-mode bits on it. (The
+     * motivation is that we want to set those file-mode bits independently
+     * of the current umask.)
+     *
+     * If the exclusive creation fails because the file already exists
+     * (EEXIST is not possible for any other reason), we just attempt to
+     * open the file, but in this case we won't be allowed to change the
+     * file-mode bits on the preexistent file.
+     *
+     * The pathname should never disappear between the two open()s in
+     * practice. If it happens, then someone very likely tried to race us.
+     * In this case just go ahead and report the ENOENT from the second
+     * open() to the caller.
+     *
+     * If the caller wants to open a preexistent file, then the first
+     * open() is decisive and its third argument is ignored, and the second
+     * open() and the fchmod() are never called.
+     */
+    fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+    if (fd == -1 && errno == EEXIST) {
+        oflag &= ~(unsigned)O_CREAT;
+        fd = open(path, oflag);
+    }
+    if (fd == -1) {
+        error_setg_errno(errp, errno,
+                         "failed to open file '%s' "
+                         "(mode: '%s')",
+                         path, mode);
+        goto end;
+    }
 
-    oflag = find_open_flag(mode, &local_err);
-    if (local_err == NULL) {
-        int fd;
-
-        /* If the caller wants / allows creation of a new file, we implement it
-         * with a two step process: open() + (open() / fchmod()).
-         *
-         * First we insist on creating the file exclusively as a new file. If
-         * that succeeds, we're free to set any file-mode bits on it. (The
-         * motivation is that we want to set those file-mode bits independently
-         * of the current umask.)
-         *
-         * If the exclusive creation fails because the file already exists
-         * (EEXIST is not possible for any other reason), we just attempt to
-         * open the file, but in this case we won't be allowed to change the
-         * file-mode bits on the preexistent file.
-         *
-         * The pathname should never disappear between the two open()s in
-         * practice. If it happens, then someone very likely tried to race us.
-         * In this case just go ahead and report the ENOENT from the second
-         * open() to the caller.
-         *
-         * If the caller wants to open a preexistent file, then the first
-         * open() is decisive and its third argument is ignored, and the second
-         * open() and the fchmod() are never called.
-         */
-        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
-        if (fd == -1 && errno == EEXIST) {
-            oflag &= ~(unsigned)O_CREAT;
-            fd = open(path, oflag);
-        }
+    qemu_set_cloexec(fd);
 
-        if (fd == -1) {
-            error_setg_errno(&local_err, errno, "failed to open file '%s' "
-                             "(mode: '%s')", path, mode);
-        } else {
-            qemu_set_cloexec(fd);
+    if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+        error_setg_errno(errp, errno,
+                         "failed to set permission 0%03o on new file '%s' (mode: '%s')",
+                         (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+        goto end;
+    }
 
-            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
-                error_setg_errno(&local_err, errno, "failed to set permission "
-                                 "0%03o on new file '%s' (mode: '%s')",
-                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
-            } else {
-                FILE *f;
-
-                f = fdopen(fd, mode);
-                if (f == NULL) {
-                    error_setg_errno(&local_err, errno, "failed to associate "
-                                     "stdio stream with file descriptor %d, "
-                                     "file '%s' (mode: '%s')", fd, path, mode);
-                } else {
-                    return f;
-                }
-            }
+    f = fdopen(fd, mode);
+    if (f == NULL) {
+        error_setg_errno(errp, errno,
+                         "failed to associate stdio stream with file descriptor %d, "
+                         "file '%s' (mode: '%s')",
+                         fd, path, mode);
+    }
 
-            close(fd);
-            if (oflag & O_CREAT) {
-                unlink(path);
-            }
+end:
+    if (f == NULL && fd != -1) {
+        close(fd);
+        if (oflag & O_CREAT) {
+            unlink(path);
         }
     }
-
-    error_propagate(errp, local_err);
-    return NULL;
+    return f;
 }
 
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (3 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 04/15] qga: flatten safe_open_or_create() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-23 12:01   ` Markus Armbruster
                     ` (2 more replies)
  2022-05-13 18:08 ` [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
                   ` (10 subsequent siblings)
  15 siblings, 3 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Used in the next patch, to simplify qga code.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 67cc465416..64f51cfb7a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
  */
 int qemu_open_old(const char *name, int flags, ...);
 int qemu_open(const char *name, int flags, Error **errp);
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe..545a88e1fd 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
 }
 #endif
 
-static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
 {
+    ERRP_GUARD();
     int ret;
+
 #ifdef O_CLOEXEC
     ret = open(name, flags | O_CLOEXEC, mode);
 #else
@@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
         qemu_set_cloexec(ret);
     }
 #endif
+    if (ret == -1) {
+        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
+    }
+
     return ret;
 }
 
@@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
     }
 #endif
 
-    ret = qemu_open_cloexec(name, flags, mode);
+    ret = qemu_open_cloexec(name, flags, mode, NULL);
 
     if (ret == -1) {
         const char *action = flags & O_CREAT ? "create" : "open";
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (4 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-16  7:24   ` Markus Armbruster
  2022-05-13 18:08 ` [PATCH v3 07/15] qga: throw an Error in ga_channel_open() marcandre.lureau
                   ` (9 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

The function takes care of setting CLOEXEC, and reporting error.

The reported error message will differ, from:
  "failed to open file 'foo' (mode: 'r')"
to:
  "Failed to open file 'foo'"

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 15eb7cb77d..7761458ce1 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -339,6 +339,7 @@ find_open_flag(const char *mode_str, Error **errp)
 static FILE *
 safe_open_or_create(const char *path, const char *mode, Error **errp)
 {
+    ERRP_GUARD();
     int oflag;
     int fd = -1;
     FILE *f = NULL;
@@ -370,21 +371,17 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
      * open() is decisive and its third argument is ignored, and the second
      * open() and the fchmod() are never called.
      */
-    fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+    fd = qemu_open_cloexec(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0, errp);
     if (fd == -1 && errno == EEXIST) {
+        error_free(*errp);
+        *errp = NULL;
         oflag &= ~(unsigned)O_CREAT;
-        fd = open(path, oflag);
+        fd = qemu_open_cloexec(path, oflag, 0, errp);
     }
     if (fd == -1) {
-        error_setg_errno(errp, errno,
-                         "failed to open file '%s' "
-                         "(mode: '%s')",
-                         path, mode);
         goto end;
     }
 
-    qemu_set_cloexec(fd);
-
     if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
         error_setg_errno(errp, errno,
                          "failed to set permission 0%03o on new file '%s' (mode: '%s')",
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 07/15] qga: throw an Error in ga_channel_open()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (5 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-16  7:27   ` Markus Armbruster
  2022-05-13 18:08 ` [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
                   ` (8 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Allow for a single point of error reporting, and further refactoring.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/channel-posix.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index a996858e24..039e1ddcb2 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int fd)
 }
 
 static gboolean ga_channel_open(GAChannel *c, const gchar *path,
-                                GAChannelMethod method, int fd)
+                                GAChannelMethod method, int fd, Error **errp)
 {
+    ERRP_GUARD();
     int ret;
     c->method = method;
 
@@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
 #endif
                            );
         if (fd == -1) {
-            g_critical("error opening channel: %s", strerror(errno));
+            error_setg_errno(errp, errno, "error opening channel");
             return false;
         }
 #ifdef CONFIG_SOLARIS
         ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
         if (ret == -1) {
-            g_critical("error setting event mask for channel: %s",
-                       strerror(errno));
+            error_setg_errno(errp, errno, "error setting event mask for channel");
             close(fd);
             return false;
         }
 #endif
         ret = ga_channel_client_add(c, fd);
         if (ret) {
-            g_critical("error adding channel to main loop");
+            error_setg(errp, "error adding channel to main loop");
             close(fd);
             return false;
         }
@@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
         assert(fd < 0);
         fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
         if (fd == -1) {
-            g_critical("error opening channel: %s", strerror(errno));
+            error_setg_errno(errp, errno, "error opening channel");
             return false;
         }
         tcgetattr(fd, &tio);
@@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
         tcsetattr(fd, TCSANOW, &tio);
         ret = ga_channel_client_add(c, fd);
         if (ret) {
-            g_critical("error adding channel to main loop");
+            error_setg(errp, "error adding channel to main loop");
             close(fd);
             return false;
         }
@@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
     }
     case GA_CHANNEL_UNIX_LISTEN: {
         if (fd < 0) {
-            Error *local_err = NULL;
-
-            fd = unix_listen(path, &local_err);
-            if (local_err != NULL) {
-                g_critical("%s", error_get_pretty(local_err));
-                error_free(local_err);
+            fd = unix_listen(path, errp);
+            if (fd < 0) {
                 return false;
             }
         }
@@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
     }
     case GA_CHANNEL_VSOCK_LISTEN: {
         if (fd < 0) {
-            Error *local_err = NULL;
             SocketAddress *addr;
             char *addr_str;
 
             addr_str = g_strdup_printf("vsock:%s", path);
-            addr = socket_parse(addr_str, &local_err);
+            addr = socket_parse(addr_str, errp);
             g_free(addr_str);
-            if (local_err != NULL) {
-                g_critical("%s", error_get_pretty(local_err));
-                error_free(local_err);
+            if (!addr) {
                 return false;
             }
 
-            fd = socket_listen(addr, 1, &local_err);
+            fd = socket_listen(addr, 1, errp);
             qapi_free_SocketAddress(addr);
-            if (local_err != NULL) {
-                g_critical("%s", error_get_pretty(local_err));
-                error_free(local_err);
+            if (fd < 0) {
                 return false;
             }
         }
@@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
         break;
     }
     default:
-        g_critical("error binding/listening to specified socket");
+        error_setg(errp, "error binding/listening to specified socket");
         return false;
     }
 
@@ -272,12 +263,14 @@ GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count)
 GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
                           int listen_fd, GAChannelCallback cb, gpointer opaque)
 {
+    Error *err = NULL;
     GAChannel *c = g_new0(GAChannel, 1);
     c->event_cb = cb;
     c->user_data = opaque;
 
-    if (!ga_channel_open(c, path, method, listen_fd)) {
-        g_critical("error opening channel");
+    if (!ga_channel_open(c, path, method, listen_fd, &err)) {
+        g_critical("%s", error_get_pretty(err));
+        error_free(err);
         ga_channel_free(c);
         return NULL;
     }
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec()
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (6 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 07/15] qga: throw an Error in ga_channel_open() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-20 13:39   ` Konstantin Kostiuk
  2022-05-13 18:08 ` [PATCH v3 09/15] qga: make build_fs_mount_list() return a bool marcandre.lureau
                   ` (7 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

qemu_open_old() uses qemu_open_internal() which handles special
"/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
reporting (and some O_DIRECT special error casing).

The monitor fdset handling is unnecessary for qga, use
qemu_open_cloexec() instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/channel-posix.c  | 14 +++++++++-----
 qga/commands-posix.c | 24 ++++++++++++------------
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 039e1ddcb2..affee485fc 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include <termios.h>
 #include "qapi/error.h"
 #include "qemu/sockets.h"
@@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
     switch (c->method) {
     case GA_CHANNEL_VIRTIO_SERIAL: {
         assert(fd < 0);
-        fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
+        fd = qemu_open_cloexec(
+            path,
 #ifndef CONFIG_SOLARIS
-                           | O_ASYNC
+            O_ASYNC |
 #endif
-                           );
+            O_RDWR | O_NONBLOCK,
+            0,
+            errp
+        );
         if (fd == -1) {
             error_setg_errno(errp, errno, "error opening channel");
             return false;
@@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
         struct termios tio;
 
         assert(fd < 0);
-        fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
+        fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0, errp);
         if (fd == -1) {
-            error_setg_errno(errp, errno, "error opening channel");
             return false;
         }
         tcgetattr(fd, &tio);
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7761458ce1..e8eafad74d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1394,6 +1394,7 @@ static GuestDiskInfoList *get_disk_partitions(
 
 static void get_nvme_smart(GuestDiskInfo *disk)
 {
+    Error *err = NULL;
     int fd;
     GuestNVMeSmart *smart;
     NvmeSmartLog log = {0};
@@ -1406,9 +1407,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
                  | (((sizeof(log) >> 2) - 1) << 16)
     };
 
-    fd = qemu_open_old(disk->name, O_RDONLY);
+    fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, &err);
     if (fd == -1) {
-        g_debug("Failed to open device: %s: %s", disk->name, g_strerror(errno));
+        g_debug("Failed to open device: %s: %s", disk->name, error_get_pretty(err));
+        error_free(err);
         return;
     }
 
@@ -1739,9 +1741,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
             }
         }
 
-        fd = qemu_open_old(mount->dirname, O_RDONLY);
+        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
         if (fd == -1) {
-            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
             goto error;
         }
 
@@ -1806,7 +1807,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 
     QTAILQ_FOREACH(mount, &mounts, next) {
         logged = false;
-        fd = qemu_open_old(mount->dirname, O_RDONLY);
+        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
         if (fd == -1) {
             continue;
         }
@@ -1866,21 +1867,20 @@ static void guest_fsfreeze_cleanup(void)
 GuestFilesystemTrimResponse *
 qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
+    ERRP_GUARD();
     GuestFilesystemTrimResponse *response;
     GuestFilesystemTrimResult *result;
     int ret = 0;
     FsMountList mounts;
     struct FsMount *mount;
     int fd;
-    Error *local_err = NULL;
     struct fstrim_range r;
 
     slog("guest-fstrim called");
 
     QTAILQ_INIT(&mounts);
-    build_fs_mount_list(&mounts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    build_fs_mount_list(&mounts, errp);
+    if (*errp) {
         return NULL;
     }
 
@@ -1892,11 +1892,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 
         QAPI_LIST_PREPEND(response->paths, result);
 
-        fd = qemu_open_old(mount->dirname, O_RDONLY);
+        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
         if (fd == -1) {
-            result->error = g_strdup_printf("failed to open: %s",
-                                            strerror(errno));
+            result->error = g_strdup(error_get_pretty(*errp));
             result->has_error = true;
+            g_clear_pointer(errp, error_free);
             continue;
         }
 
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 09/15] qga: make build_fs_mount_list() return a bool
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (7 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Change build_fs_mount_list() to return bool, in accordance
with the guidance under = Rules = in include/qapi/error.h

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-posix.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e8eafad74d..a414875109 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -673,7 +673,7 @@ static int dev_major_minor(const char *devpath,
 /*
  * Walk the mount table and build a list of local file systems
  */
-static void build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
+static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
 {
     struct mntent *ment;
     FsMount *mount;
@@ -684,7 +684,7 @@ static void build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
     fp = setmntent(mtab, "r");
     if (!fp) {
         error_setg(errp, "failed to open mtab file: '%s'", mtab);
-        return;
+        return false;
     }
 
     while ((ment = getmntent(fp))) {
@@ -714,6 +714,7 @@ static void build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
     }
 
     endmntent(fp);
+    return true;
 }
 
 static void decode_mntname(char *name, int len)
@@ -738,7 +739,7 @@ static void decode_mntname(char *name, int len)
     }
 }
 
-static void build_fs_mount_list(FsMountList *mounts, Error **errp)
+static bool build_fs_mount_list(FsMountList *mounts, Error **errp)
 {
     FsMount *mount;
     char const *mountinfo = "/proc/self/mountinfo";
@@ -751,8 +752,7 @@ static void build_fs_mount_list(FsMountList *mounts, Error **errp)
 
     fp = fopen(mountinfo, "r");
     if (!fp) {
-        build_fs_mount_list_from_mtab(mounts, errp);
-        return;
+        return build_fs_mount_list_from_mtab(mounts, errp);
     }
 
     while (getline(&line, &n, fp) != -1) {
@@ -794,6 +794,7 @@ static void build_fs_mount_list(FsMountList *mounts, Error **errp)
     free(line);
 
     fclose(fp);
+    return true;
 }
 #endif
 
@@ -1594,8 +1595,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
     Error *local_err = NULL;
 
     QTAILQ_INIT(&mounts);
-    build_fs_mount_list(&mounts, &local_err);
-    if (local_err) {
+    if (!build_fs_mount_list(&mounts, &local_err)) {
         error_propagate(errp, local_err);
         return NULL;
     }
@@ -1718,8 +1718,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
     }
 
     QTAILQ_INIT(&mounts);
-    build_fs_mount_list(&mounts, &local_err);
-    if (local_err) {
+    if (!build_fs_mount_list(&mounts, &local_err)) {
         error_propagate(errp, local_err);
         return -1;
     }
@@ -1799,8 +1798,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
     Error *local_err = NULL;
 
     QTAILQ_INIT(&mounts);
-    build_fs_mount_list(&mounts, &local_err);
-    if (local_err) {
+    if (!build_fs_mount_list(&mounts, &local_err)) {
         error_propagate(errp, local_err);
         return 0;
     }
@@ -1879,8 +1877,7 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
     slog("guest-fstrim called");
 
     QTAILQ_INIT(&mounts);
-    build_fs_mount_list(&mounts, errp);
-    if (*errp) {
+    if (!build_fs_mount_list(&mounts, errp)) {
         return NULL;
     }
 
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (8 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 09/15] qga: make build_fs_mount_list() return a bool marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-20 13:43   ` Konstantin Kostiuk
  2022-05-13 18:08 ` [PATCH v3 11/15] qga/wixl: prefer variables over environment marcandre.lureau
                   ` (5 subsequent siblings)
  15 siblings, 1 reply; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

This a more accurate way to lookup the test data, and will allow to move
the test in a subproject.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-qga.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index d6df1ee92e..ab0b12a2dd 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -914,15 +914,14 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
     const gchar *str;
-    gchar *cwd, *env[2];
-    QDict *ret, *val;
+    QDict *ret = NULL;
+    char *env[2];
+    QDict *val;
 
-    cwd = g_get_current_dir();
     env[0] = g_strdup_printf(
-        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
-        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
+        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
-    g_free(cwd);
     fixture_setup(&fixture, NULL, env);
 
     ret = qmp_fd(fixture.fd, "{'execute': 'guest-get-osinfo'}");
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 11/15] qga/wixl: prefer variables over environment
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (9 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 12/15] qga/wixl: require Mingw_bin marcandre.lureau
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

No need to setup an environment or to check if the variable is undefined
manually.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/installer/qemu-ga.wxs | 30 +++++++++---------------------
 qga/meson.build           |  9 ++++-----
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 0950e8c6be..8a19aa1656 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -1,17 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Wix xmlns="http://schemas.microsoft.com/wix/2006/wi">
-  <?ifndef env.QEMU_GA_VERSION ?>
-    <?error Environment variable QEMU_GA_VERSION undefined?>
-  <?endif?>
-
-  <?ifndef env.QEMU_GA_DISTRO ?>
-    <?error Environment variable QEMU_GA_DISTRO undefined?>
-  <?endif?>
-
-  <?ifndef env.QEMU_GA_MANUFACTURER ?>
-    <?error Environment variable QEMU_GA_MANUFACTURER undefined?>
-  <?endif?>
-
   <?ifndef var.Arch?>
     <?error Define Arch to 32 or 64?>
   <?endif?>
@@ -43,20 +31,20 @@
     Name="QEMU guest agent"
     Id="*"
     UpgradeCode="{EB6B8302-C06E-4BEC-ADAC-932C68A3A98D}"
-    Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
-    Version="$(env.QEMU_GA_VERSION)"
+    Manufacturer="$(var.QEMU_GA_MANUFACTURER)"
+    Version="$(var.QEMU_GA_VERSION)"
     Language="1033">
     <?if $(var.Arch) = 32 ?>
     <Condition Message="Error: 32-bit version of Qemu GA can not be installed on 64-bit Windows.">NOT VersionNT64</Condition>
     <?endif?>
     <Package
-      Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
+      Manufacturer="$(var.QEMU_GA_MANUFACTURER)"
       InstallerVersion="200"
       Languages="1033"
       Compressed="yes"
       InstallScope="perMachine"
       />
-    <Media Id="1" Cabinet="qemu_ga.$(env.QEMU_GA_VERSION).cab" EmbedCab="yes" />
+    <Media Id="1" Cabinet="qemu_ga.$(var.QEMU_GA_VERSION).cab" EmbedCab="yes" />
     <Property Id="WHSLogo">1</Property>
     <MajorUpgrade
       DowngradeErrorMessage="Error: A newer version of QEMU guest agent is already installed."
@@ -66,7 +54,7 @@
       <Directory Id="$(var.GaProgramFilesFolder)" Name="QEMU Guest Agent">
         <Directory Id="qemu_ga_directory" Name="Qemu-ga">
           <Component Id="qemu_ga" Guid="{908B7199-DE2A-4DC6-A8D0-27A5AE444FEA}">
-            <File Id="qemu_ga.exe" Name="qemu-ga.exe" Source="$(env.BUILD_DIR)/qga/qemu-ga.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="qemu_ga.exe" Name="qemu-ga.exe" Source="$(var.BUILD_DIR)/qga/qemu-ga.exe" KeyPath="yes" DiskId="1"/>
             <ServiceInstall
               Id="ServiceInstaller"
               Type="ownProcess"
@@ -88,10 +76,10 @@
             <File Id="libstdc++-6.lib" Name="libstdc++-6.dll" Source="$(var.Mingw_bin)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
-            <File Id="qga_vss.dll" Name="qga-vss.dll" Source="$(env.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="qga_vss.dll" Name="qga-vss.dll" Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="qga_vss_tlb" Guid="{D8D584B1-59C2-4FB7-A91F-636FF7BFA66E}">
-            <File Id="qga_vss.tlb" Name="qga-vss.tlb" Source="$(env.BUILD_DIR)/qga/vss-win32/qga-vss.tlb" KeyPath="yes" DiskId="1"/>
+            <File Id="qga_vss.tlb" Name="qga-vss.tlb" Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.tlb" KeyPath="yes" DiskId="1"/>
           </Component>
           <?endif?>
           <?if $(var.Arch) = "32"?>
@@ -133,9 +121,9 @@
           </Component>
           <Component Id="registry_entries" Guid="{D075D109-51CA-11E3-9F8B-000C29858960}">
             <RegistryKey Root="HKLM"
-                         Key="Software\$(env.QEMU_GA_MANUFACTURER)\$(env.QEMU_GA_DISTRO)\Tools\QemuGA">
+                         Key="Software\$(var.QEMU_GA_MANUFACTURER)\$(var.QEMU_GA_DISTRO)\Tools\QemuGA">
               <RegistryValue Type="string" Name="ProductID" Value="fb0a0d66-c7fb-4e2e-a16b-c4a3bfe8d13b" />
-              <RegistryValue Type="string" Name="Version" Value="$(env.QEMU_GA_VERSION)" />
+              <RegistryValue Type="string" Name="Version" Value="$(var.QEMU_GA_VERSION)" />
             </RegistryKey>
           </Component>
         </Directory>
diff --git a/qga/meson.build b/qga/meson.build
index 6d9f39bb32..3ad3bc0260 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -121,15 +121,14 @@ if targetos == 'windows'
                             output: 'qemu-ga-@0@.msi'.format(host_arch),
                             depends: deps,
                             command: [
-                              find_program('env'),
-                              'QEMU_GA_VERSION=' + config_host['QEMU_GA_VERSION'],
-                              'QEMU_GA_MANUFACTURER=' + config_host['QEMU_GA_MANUFACTURER'],
-                              'QEMU_GA_DISTRO=' + config_host['QEMU_GA_DISTRO'],
-                              'BUILD_DIR=' + meson.build_root(),
                               wixl, '-o', '@OUTPUT0@', '@INPUT0@',
                               qemu_ga_msi_arch[cpu],
                               qemu_ga_msi_vss,
+                              '-D', 'BUILD_DIR=' + meson.build_root(),
                               '-D', 'Mingw_bin=' + config_host['QEMU_GA_MSI_MINGW_BIN_PATH'],
+                              '-D', 'QEMU_GA_VERSION=' + config_host['QEMU_GA_VERSION'],
+                              '-D', 'QEMU_GA_MANUFACTURER=' + config_host['QEMU_GA_MANUFACTURER'],
+                              '-D', 'QEMU_GA_DISTRO=' + config_host['QEMU_GA_DISTRO'],
                             ])
     all_qga += [qga_msi]
     alias_target('msi', qga_msi)
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 12/15] qga/wixl: require Mingw_bin
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (10 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 11/15] qga/wixl: prefer variables over environment marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

No clear reason to make guesses here.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/installer/qemu-ga.wxs | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 8a19aa1656..651db6e51c 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -4,15 +4,6 @@
     <?error Define Arch to 32 or 64?>
   <?endif?>
 
-  <?ifndef var.Mingw_bin?>
-    <?if $(var.Arch) = "64"?>
-      <?define Mingw_bin=/usr/x86_64-w64-mingw32/sys-root/mingw/bin ?>
-    <?endif?>
-    <?if $(var.Arch) = "32"?>
-      <?define Mingw_bin=/usr/i686-w64-mingw32/sys-root/mingw/bin ?>
-    <?endif?>
-  <?endif?>
-
   <?if $(var.Arch) = "64"?>
     <?define ArchLib=libgcc_s_seh-1.dll?>
     <?define GaProgramFilesFolder="ProgramFiles64Folder" ?>
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 13/15] qga/wixl: simplify some pre-processing
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (11 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 12/15] qga/wixl: require Mingw_bin marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Sadly, wixl doesn't have 'elif'.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/installer/qemu-ga.wxs | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 651db6e51c..e5b0958e18 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -1,21 +1,15 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Wix xmlns="http://schemas.microsoft.com/wix/2006/wi">
-  <?ifndef var.Arch?>
-    <?error Define Arch to 32 or 64?>
-  <?endif?>
-
   <?if $(var.Arch) = "64"?>
     <?define ArchLib=libgcc_s_seh-1.dll?>
     <?define GaProgramFilesFolder="ProgramFiles64Folder" ?>
-  <?endif?>
-
-  <?if $(var.Arch) = "32"?>
-    <?define ArchLib=libgcc_s_dw2-1.dll?>
-    <?define GaProgramFilesFolder="ProgramFilesFolder" ?>
-  <?endif?>
-
-  <?ifndef var.ArchLib ?>
-    <?error Unexpected Arch value $(var.Arch)?>
+  <?else?>
+    <?if $(var.Arch) = "32"?>
+      <?define ArchLib=libgcc_s_dw2-1.dll?>
+      <?define GaProgramFilesFolder="ProgramFilesFolder" ?>
+    <?else?>
+      <?error Unexpected Arch value $(var.Arch)?>
+    <?endif?>
   <?endif?>
 
   <Product
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (12 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-13 18:08 ` [PATCH v3 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
  2022-05-19 17:05 ` [PATCH v3 00/15] Misc cleanups Marc-André Lureau
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Use more conventional variables to set the location of pre-built
DLL/bin.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 configure                 |  9 ++++++---
 meson.build               |  5 ++++-
 qga/installer/qemu-ga.wxs | 24 ++++++++++++------------
 qga/meson.build           |  2 +-
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/configure b/configure
index c8b5b99532..fb9b4657ae 100755
--- a/configure
+++ b/configure
@@ -1507,6 +1507,11 @@ for i in $glib_modules; do
     fi
 done
 
+glib_bindir="$($pkg_config --variable=bindir glib-2.0)"
+if test -z "$glib_bindir" ; then
+	glib_bindir="$($pkg_config --variable=prefix glib-2.0)"/bin
+fi
+
 # This workaround is required due to a bug in pkg-config file for glib as it
 # doesn't define GLIB_STATIC_COMPILATION for pkg-config --static
 
@@ -1882,8 +1887,6 @@ if test "$QEMU_GA_VERSION" = ""; then
     QEMU_GA_VERSION=$(cat $source_path/VERSION)
 fi
 
-QEMU_GA_MSI_MINGW_BIN_PATH="$($pkg_config --variable=prefix glib-2.0)/bin"
-
 # Mac OS X ships with a broken assembler
 roms=
 if { test "$cpu" = "i386" || test "$cpu" = "x86_64"; } && \
@@ -1970,7 +1973,6 @@ if test "$debug_tcg" = "yes" ; then
 fi
 if test "$mingw32" = "yes" ; then
   echo "CONFIG_WIN32=y" >> $config_host_mak
-  echo "QEMU_GA_MSI_MINGW_BIN_PATH=${QEMU_GA_MSI_MINGW_BIN_PATH}" >> $config_host_mak
   echo "QEMU_GA_MANUFACTURER=${QEMU_GA_MANUFACTURER}" >> $config_host_mak
   echo "QEMU_GA_DISTRO=${QEMU_GA_DISTRO}" >> $config_host_mak
   echo "QEMU_GA_VERSION=${QEMU_GA_VERSION}" >> $config_host_mak
@@ -2043,6 +2045,7 @@ echo "QEMU_CXXFLAGS=$QEMU_CXXFLAGS" >> $config_host_mak
 echo "QEMU_OBJCFLAGS=$QEMU_OBJCFLAGS" >> $config_host_mak
 echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
 echo "GLIB_LIBS=$glib_libs" >> $config_host_mak
+echo "GLIB_BINDIR=$glib_bindir" >> $config_host_mak
 echo "GLIB_VERSION=$(pkg-config --modversion glib-2.0)" >> $config_host_mak
 echo "QEMU_LDFLAGS=$QEMU_LDFLAGS" >> $config_host_mak
 echo "LD_I386_EMULATION=$ld_i386_emulation" >> $config_host_mak
diff --git a/meson.build b/meson.build
index 9b20dcd143..3e5040fe0b 100644
--- a/meson.build
+++ b/meson.build
@@ -466,7 +466,10 @@ add_project_arguments(config_host['GLIB_CFLAGS'].split(),
                       native: false, language: ['c', 'cpp', 'objc'])
 glib = declare_dependency(compile_args: config_host['GLIB_CFLAGS'].split(),
                           link_args: config_host['GLIB_LIBS'].split(),
-                          version: config_host['GLIB_VERSION'])
+                          version: config_host['GLIB_VERSION'],
+                          variables: {
+                            'bindir': config_host['GLIB_BINDIR'],
+                          })
 # override glib dep with the configure results (for subprojects)
 meson.override_dependency('glib-2.0', glib)
 
diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index e5b0958e18..813d1c6ca6 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -58,7 +58,7 @@
           </Component>
           <?ifdef var.InstallVss?>
           <Component Id="libstdc++_6_lib" Guid="{55E737B5-9127-4A11-9FC3-A29367714574}">
-            <File Id="libstdc++-6.lib" Name="libstdc++-6.dll" Source="$(var.Mingw_bin)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libstdc++-6.lib" Name="libstdc++-6.dll" Source="$(var.BIN_DIR)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
             <File Id="qga_vss.dll" Name="qga-vss.dll" Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes" DiskId="1"/>
@@ -69,40 +69,40 @@
           <?endif?>
           <?if $(var.Arch) = "32"?>
           <Component Id="gspawn-helper-console" Guid="{446185B3-87BE-43D2-96B8-0FEFD9E8696D}">
-            <File Id="gspawn-win32-helper-console.exe" Name="gspawn-win32-helper-console.exe" Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win32-helper-console.exe" Name="gspawn-win32-helper-console.exe" Source="$(var.BIN_DIR)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="gspawn-helper" Guid="{CD67A5A3-2DB1-4DA1-A67A-8D71E797B466}">
-            <File Id="gspawn-win32-helper.exe" Name="gspawn-win32-helper.exe" Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win32-helper.exe" Name="gspawn-win32-helper.exe" Source="$(var.BIN_DIR)/gspawn-win32-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <?endif?>
           <?if $(var.Arch) = "64"?>
           <Component Id="gspawn-helper-console" Guid="{9E615A9F-349A-4992-A5C2-C10BAD173660}">
-            <File Id="gspawn-win64-helper-console.exe" Name="gspawn-win64-helper-console.exe" Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win64-helper-console.exe" Name="gspawn-win64-helper-console.exe" Source="$(var.BIN_DIR)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="gspawn-helper" Guid="{D201AD22-1846-4E4F-B6E1-C7A908ED2457}">
-            <File Id="gspawn-win64-helper.exe" Name="gspawn-win64-helper.exe" Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
+            <File Id="gspawn-win64-helper.exe" Name="gspawn-win64-helper.exe" Source="$(var.BIN_DIR)/gspawn-win64-helper-console.exe" KeyPath="yes" DiskId="1"/>
           </Component>
           <?endif?>
           <Component Id="iconv" Guid="{35EE3558-D34B-4F0A-B8BD-430FF0775246}">
-            <File Id="iconv.dll" Name="iconv.dll" Source="$(var.Mingw_bin)/iconv.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="iconv.dll" Name="iconv.dll" Source="$(var.BIN_DIR)/iconv.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libgcc_arch_lib" Guid="{ADD4D07D-4515-4AB6-AF3E-C904961B4BB0}">
-            <File Id="libgcc_arch_lib" Name="$(var.ArchLib)" Source="$(var.Mingw_bin)/$(var.ArchLib)" KeyPath="yes" DiskId="1"/>
+            <File Id="libgcc_arch_lib" Name="$(var.ArchLib)" Source="$(var.BIN_DIR)/$(var.ArchLib)" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libglib" Guid="{D31BFD83-2773-4B65-B45A-E0D2ADA58679}">
-            <File Id="libglib_2.0_0.dll" Name="libglib-2.0-0.dll" Source="$(var.Mingw_bin)/libglib-2.0-0.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libglib_2.0_0.dll" Name="libglib-2.0-0.dll" Source="$(var.BIN_DIR)/libglib-2.0-0.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libintl" Guid="{A641BC2D-A907-4A94-9149-F30ED430878F}">
-            <File Id="libintl_8.dll" Name="libintl-8.dll" Source="$(var.Mingw_bin)/libintl-8.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libintl_8.dll" Name="libintl-8.dll" Source="$(var.BIN_DIR)/libintl-8.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libssp" Guid="{7880087B-02B4-4EF6-A5D3-D18F8E3D90E1}">
-            <File Id="libssp_0.dll" Name="libssp-0.dll" Source="$(var.Mingw_bin)/libssp-0.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libssp_0.dll" Name="libssp-0.dll" Source="$(var.BIN_DIR)/libssp-0.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libwinpthread" Guid="{6C117C78-0F47-4B07-8F34-6BEE11643829}">
-            <File Id="libwinpthread_1.dll" Name="libwinpthread-1.dll" Source="$(var.Mingw_bin)/libwinpthread-1.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libwinpthread_1.dll" Name="libwinpthread-1.dll" Source="$(var.BIN_DIR)/libwinpthread-1.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="libpcre" Guid="{7A86B45E-A009-489A-A849-CE3BACF03CD0}">
-            <File Id="libpcre_1.dll" Name="libpcre-1.dll" Source="$(var.Mingw_bin)/libpcre-1.dll" KeyPath="yes" DiskId="1"/>
+            <File Id="libpcre_1.dll" Name="libpcre-1.dll" Source="$(var.BIN_DIR)/libpcre-1.dll" KeyPath="yes" DiskId="1"/>
           </Component>
           <Component Id="registry_entries" Guid="{D075D109-51CA-11E3-9F8B-000C29858960}">
             <RegistryKey Root="HKLM"
diff --git a/qga/meson.build b/qga/meson.build
index 3ad3bc0260..51b1e611b1 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -125,7 +125,7 @@ if targetos == 'windows'
                               qemu_ga_msi_arch[cpu],
                               qemu_ga_msi_vss,
                               '-D', 'BUILD_DIR=' + meson.build_root(),
-                              '-D', 'Mingw_bin=' + config_host['QEMU_GA_MSI_MINGW_BIN_PATH'],
+                              '-D', 'BIN_DIR=' + glib.get_variable('bindir'),
                               '-D', 'QEMU_GA_VERSION=' + config_host['QEMU_GA_VERSION'],
                               '-D', 'QEMU_GA_MANUFACTURER=' + config_host['QEMU_GA_MANUFACTURER'],
                               '-D', 'QEMU_GA_DISTRO=' + config_host['QEMU_GA_DISTRO'],
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 15/15] test/qga: use g_auto wherever sensible
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (13 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
@ 2022-05-13 18:08 ` marcandre.lureau
  2022-05-19 17:05 ` [PATCH v3 00/15] Misc cleanups Marc-André Lureau
  15 siblings, 0 replies; 31+ messages in thread
From: marcandre.lureau @ 2022-05-13 18:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster,
	Marc-André Lureau

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-qga.c | 121 +++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 78 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index ab0b12a2dd..530317044b 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -52,7 +52,10 @@ fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
 {
     const gchar *extra_arg = data;
     GError *error = NULL;
-    gchar *cwd, *path, *cmd, **argv = NULL;
+    g_autofree char *cwd = NULL;
+    g_autofree char *path = NULL;
+    g_autofree char *cmd = NULL;
+    g_auto(GStrv) argv = NULL;
 
     fixture->loop = g_main_loop_new(NULL, FALSE);
 
@@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
 
     fixture->fd = connect_qga(path);
     g_assert_cmpint(fixture->fd, !=, -1);
-
-    g_strfreev(argv);
-    g_free(cmd);
-    g_free(cwd);
-    g_free(path);
 }
 
 static void
 fixture_tear_down(TestFixture *fixture, gconstpointer data)
 {
-    gchar *tmp;
+    g_autofree char *tmp = NULL;
 
     kill(fixture->pid, SIGTERM);
 
@@ -107,7 +105,6 @@ fixture_tear_down(TestFixture *fixture, gconstpointer data)
 
     tmp = g_build_filename(fixture->test_dir, "sock", NULL);
     g_unlink(tmp);
-    g_free(tmp);
 
     g_rmdir(fixture->test_dir);
     g_free(fixture->test_dir);
@@ -122,7 +119,7 @@ static void qmp_assertion_message_error(const char     *domain,
                                         QDict          *dict)
 {
     const char *class, *desc;
-    char *s;
+    g_autofree char *s = NULL;
     QDict *error;
 
     error = qdict_get_qdict(dict, "error");
@@ -131,7 +128,6 @@ static void qmp_assertion_message_error(const char     *domain,
 
     s = g_strdup_printf("assertion failed %s: %s %s", expr, class, desc);
     g_assertion_message(domain, file, line, func, s);
-    g_free(s);
 }
 
 #define qmp_assert_no_error(err) do {                                   \
@@ -146,7 +142,7 @@ static void test_qga_sync_delimited(gconstpointer fix)
     const TestFixture *fixture = fix;
     guint32 v, r = g_test_rand_int();
     unsigned char c;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
 
     qmp_fd_send_raw(fixture->fd, "\xff");
     qmp_fd_send(fixture->fd,
@@ -180,15 +176,13 @@ static void test_qga_sync_delimited(gconstpointer fix)
 
     v = qdict_get_int(ret, "return");
     g_assert_cmpint(r, ==, v);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_sync(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
     guint32 v, r = g_test_rand_int();
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
 
     /*
      * TODO guest-sync is inherently limited: we cannot distinguish
@@ -210,33 +204,27 @@ static void test_qga_sync(gconstpointer fix)
 
     v = qdict_get_int(ret, "return");
     g_assert_cmpint(r, ==, v);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_ping(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping'}");
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_id(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
     g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_invalid_oob(gconstpointer fix)
@@ -253,7 +241,8 @@ static void test_qga_invalid_oob(gconstpointer fix)
 static void test_qga_invalid_args(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *error;
+    g_autoptr(QDict) ret = NULL;
+    QDict *error;
     const gchar *class, *desc;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', "
@@ -266,14 +255,13 @@ static void test_qga_invalid_args(gconstpointer fix)
 
     g_assert_cmpstr(class, ==, "GenericError");
     g_assert_cmpstr(desc, ==, "Parameter 'foo' is unexpected");
-
-    qobject_unref(ret);
 }
 
 static void test_qga_invalid_cmd(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *error;
+    g_autoptr(QDict) ret = NULL;
+    QDict *error;
     const gchar *class, *desc;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-invalid-cmd'}");
@@ -285,14 +273,13 @@ static void test_qga_invalid_cmd(gconstpointer fix)
 
     g_assert_cmpstr(class, ==, "CommandNotFound");
     g_assert_cmpint(strlen(desc), >, 0);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_info(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *val;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
     const gchar *version;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-info'}");
@@ -302,14 +289,12 @@ static void test_qga_info(gconstpointer fix)
     val = qdict_get_qdict(ret, "return");
     version = qdict_get_try_str(val, "version");
     g_assert_cmpstr(version, ==, QEMU_VERSION);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_get_vcpus(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     QList *list;
     const QListEntry *entry;
 
@@ -322,14 +307,12 @@ static void test_qga_get_vcpus(gconstpointer fix)
     entry = qlist_first(list);
     g_assert(qdict_haskey(qobject_to(QDict, entry->value), "online"));
     g_assert(qdict_haskey(qobject_to(QDict, entry->value), "logical-id"));
-
-    qobject_unref(ret);
 }
 
 static void test_qga_get_fsinfo(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     QList *list;
     const QListEntry *entry;
 
@@ -346,14 +329,13 @@ static void test_qga_get_fsinfo(gconstpointer fix)
         g_assert(qdict_haskey(qobject_to(QDict, entry->value), "type"));
         g_assert(qdict_haskey(qobject_to(QDict, entry->value), "disk"));
     }
-
-    qobject_unref(ret);
 }
 
 static void test_qga_get_memory_block_info(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *val;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
     int64_t size;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-memory-block-info'}");
@@ -366,14 +348,12 @@ static void test_qga_get_memory_block_info(gconstpointer fix)
         size = qdict_get_int(val, "size");
         g_assert_cmpint(size, >, 0);
     }
-
-    qobject_unref(ret);
 }
 
 static void test_qga_get_memory_blocks(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     QList *list;
     const QListEntry *entry;
 
@@ -391,14 +371,12 @@ static void test_qga_get_memory_blocks(gconstpointer fix)
             g_assert(qdict_haskey(qobject_to(QDict, entry->value), "online"));
         }
     }
-
-    qobject_unref(ret);
 }
 
 static void test_qga_network_get_interfaces(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     QList *list;
     const QListEntry *entry;
 
@@ -410,8 +388,6 @@ static void test_qga_network_get_interfaces(gconstpointer fix)
     list = qdict_get_qlist(ret, "return");
     entry = qlist_first(list);
     g_assert(qdict_haskey(qobject_to(QDict, entry->value), "name"));
-
-    qobject_unref(ret);
 }
 
 static void test_qga_file_ops(gconstpointer fix)
@@ -642,7 +618,7 @@ static void test_qga_file_write_read(gconstpointer fix)
 static void test_qga_get_time(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     int64_t time;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
@@ -651,8 +627,6 @@ static void test_qga_get_time(gconstpointer fix)
 
     time = qdict_get_int(ret, "return");
     g_assert_cmpint(time, >, 0);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_blacklist(gconstpointer data)
@@ -693,18 +667,22 @@ static void test_qga_blacklist(gconstpointer data)
 static void test_qga_config(gconstpointer data)
 {
     GError *error = NULL;
-    char *cwd, *cmd, *out, *err, *str, **strv, **argv = NULL;
+    g_autofree char *out = NULL;
+    g_autofree char *err = NULL;
+    g_autofree char *cwd = NULL;
+    g_autofree char *cmd = NULL;
+    g_auto(GStrv) argv = NULL;
+    g_auto(GStrv) strv = NULL;
+    g_autoptr(GKeyFile) kf = NULL;
+    char *str;
     char *env[2];
     int status;
     gsize n;
-    GKeyFile *kf;
 
     cwd = g_get_current_dir();
     cmd = g_strdup_printf("%s%cqga%cqemu-ga -D",
                           cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
-    g_free(cwd);
     g_shell_parse_argv(cmd, NULL, &argv, &error);
-    g_free(cmd);
     g_assert_no_error(error);
 
     env[0] = g_strdup_printf("QGA_CONF=tests%cdata%ctest-qga-config",
@@ -712,7 +690,6 @@ static void test_qga_config(gconstpointer data)
     env[1] = NULL;
     g_spawn_sync(NULL, argv, env, 0,
                  NULL, NULL, &out, &err, &status, &error);
-    g_strfreev(argv);
 
     g_assert_no_error(error);
     g_assert_cmpstr(err, ==, "");
@@ -759,18 +736,14 @@ static void test_qga_config(gconstpointer data)
     g_assert_true(g_strv_contains((const char * const *)strv,
                                   "guest-get-time"));
     g_assert_no_error(error);
-    g_strfreev(strv);
 
-    g_free(out);
-    g_free(err);
     g_free(env[0]);
-    g_key_file_free(kf);
 }
 
 static void test_qga_fsfreeze_status(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     const gchar *status;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-status'}");
@@ -779,16 +752,15 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
 
     status = qdict_get_try_str(ret, "return");
     g_assert_cmpstr(status, ==, "thawed");
-
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *val;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
     const gchar *out;
-    guchar *decoded;
+    g_autofree guchar *decoded = NULL;
     int64_t pid, now, exitcode;
     gsize len;
     bool exited;
@@ -827,14 +799,13 @@ static void test_qga_guest_exec(gconstpointer fix)
     decoded = g_base64_decode(out, &len);
     g_assert_cmpint(len, ==, 12);
     g_assert_cmpstr((char *)decoded, ==, "\" test_str \"");
-    g_free(decoded);
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_exec_invalid(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *error;
+    g_autoptr(QDict) ret = NULL;
+    QDict *error;
     const gchar *class, *desc;
 
     /* invalid command */
@@ -859,13 +830,13 @@ static void test_qga_guest_exec_invalid(gconstpointer fix)
     desc = qdict_get_str(error, "desc");
     g_assert_cmpstr(class, ==, "GenericError");
     g_assert_cmpint(strlen(desc), >, 0);
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_get_host_name(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *val;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-host-name'}");
     g_assert_nonnull(ret);
@@ -873,14 +844,13 @@ static void test_qga_guest_get_host_name(gconstpointer fix)
 
     val = qdict_get_qdict(ret, "return");
     g_assert(qdict_haskey(val, "host-name"));
-
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_get_timezone(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *val;
+    g_autoptr(QDict) ret = NULL;
+    QDict *val;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-timezone'}");
     g_assert_nonnull(ret);
@@ -889,14 +859,12 @@ static void test_qga_guest_get_timezone(gconstpointer fix)
     /* Make sure there's at least offset */
     val = qdict_get_qdict(ret, "return");
     g_assert(qdict_haskey(val, "offset"));
-
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_get_users(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret;
+    g_autoptr(QDict) ret = NULL;
     QList *val;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-users'}");
@@ -906,15 +874,13 @@ static void test_qga_guest_get_users(gconstpointer fix)
     /* There is not much to test here */
     val = qdict_get_qlist(ret, "return");
     g_assert_nonnull(val);
-
-    qobject_unref(ret);
 }
 
 static void test_qga_guest_get_osinfo(gconstpointer data)
 {
     TestFixture fixture;
     const gchar *str;
-    QDict *ret = NULL;
+    g_autoptr(QDict) ret = NULL;
     char *env[2];
     QDict *val;
 
@@ -958,7 +924,6 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
     g_assert_nonnull(str);
     g_assert_cmpstr(str, ==, "unit-test");
 
-    qobject_unref(ret);
     g_free(env[0]);
     fixture_tear_down(&fixture, NULL);
 }
-- 
2.36.1



^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 04/15] qga: flatten safe_open_or_create()
  2022-05-13 18:08 ` [PATCH v3 04/15] qga: flatten safe_open_or_create() marcandre.lureau
@ 2022-05-16  7:19   ` Markus Armbruster
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2022-05-16  7:19 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel, Konstantin Kostiuk, Michael Roth

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> There is a bit too much nesting in the function, this can be simplified
> a bit to improve readability.
>
> This also helps with the following error handling changes.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix.c | 122 ++++++++++++++++++++++---------------------
>  1 file changed, 62 insertions(+), 60 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 69f209af87..15eb7cb77d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -339,73 +339,75 @@ find_open_flag(const char *mode_str, Error **errp)
>  static FILE *
>  safe_open_or_create(const char *path, const char *mode, Error **errp)
>  {
> -    Error *local_err = NULL;
>      int oflag;
> +    int fd = -1;
> +    FILE *f = NULL;
> +
> +    oflag = find_open_flag(mode, errp);
> +    if (oflag < 0) {
> +        goto end;
> +    }
> +
> +    /* If the caller wants / allows creation of a new file, we implement it
> +     * with a two step process: open() + (open() / fchmod()).
> +     *
> +     * First we insist on creating the file exclusively as a new file. If
> +     * that succeeds, we're free to set any file-mode bits on it. (The
> +     * motivation is that we want to set those file-mode bits independently
> +     * of the current umask.)
> +     *
> +     * If the exclusive creation fails because the file already exists
> +     * (EEXIST is not possible for any other reason), we just attempt to
> +     * open the file, but in this case we won't be allowed to change the
> +     * file-mode bits on the preexistent file.
> +     *
> +     * The pathname should never disappear between the two open()s in
> +     * practice. If it happens, then someone very likely tried to race us.
> +     * In this case just go ahead and report the ENOENT from the second
> +     * open() to the caller.
> +     *
> +     * If the caller wants to open a preexistent file, then the first
> +     * open() is decisive and its third argument is ignored, and the second
> +     * open() and the fchmod() are never called.
> +     */
> +    fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> +    if (fd == -1 && errno == EEXIST) {
> +        oflag &= ~(unsigned)O_CREAT;
> +        fd = open(path, oflag);
> +    }
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno,
> +                         "failed to open file '%s' "
> +                         "(mode: '%s')",

No need to split this string.

> +                         path, mode);
> +        goto end;
> +    }
>  
> -    oflag = find_open_flag(mode, &local_err);
> -    if (local_err == NULL) {
> -        int fd;
> -
> -        /* If the caller wants / allows creation of a new file, we implement it
> -         * with a two step process: open() + (open() / fchmod()).
> -         *
> -         * First we insist on creating the file exclusively as a new file. If
> -         * that succeeds, we're free to set any file-mode bits on it. (The
> -         * motivation is that we want to set those file-mode bits independently
> -         * of the current umask.)
> -         *
> -         * If the exclusive creation fails because the file already exists
> -         * (EEXIST is not possible for any other reason), we just attempt to
> -         * open the file, but in this case we won't be allowed to change the
> -         * file-mode bits on the preexistent file.
> -         *
> -         * The pathname should never disappear between the two open()s in
> -         * practice. If it happens, then someone very likely tried to race us.
> -         * In this case just go ahead and report the ENOENT from the second
> -         * open() to the caller.
> -         *
> -         * If the caller wants to open a preexistent file, then the first
> -         * open() is decisive and its third argument is ignored, and the second
> -         * open() and the fchmod() are never called.
> -         */
> -        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> -        if (fd == -1 && errno == EEXIST) {
> -            oflag &= ~(unsigned)O_CREAT;
> -            fd = open(path, oflag);
> -        }
> +    qemu_set_cloexec(fd);
>  
> -        if (fd == -1) {
> -            error_setg_errno(&local_err, errno, "failed to open file '%s' "
> -                             "(mode: '%s')", path, mode);
> -        } else {
> -            qemu_set_cloexec(fd);
> +    if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> +        error_setg_errno(errp, errno,
> +                         "failed to set permission 0%03o on new file '%s' (mode: '%s')",

But I'd split this one.

> +                         (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> +        goto end;
> +    }
>  
> -            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> -                error_setg_errno(&local_err, errno, "failed to set permission "
> -                                 "0%03o on new file '%s' (mode: '%s')",
> -                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> -            } else {
> -                FILE *f;
> -
> -                f = fdopen(fd, mode);
> -                if (f == NULL) {
> -                    error_setg_errno(&local_err, errno, "failed to associate "
> -                                     "stdio stream with file descriptor %d, "
> -                                     "file '%s' (mode: '%s')", fd, path, mode);
> -                } else {
> -                    return f;
> -                }
> -            }
> +    f = fdopen(fd, mode);
> +    if (f == NULL) {
> +        error_setg_errno(errp, errno,
> +                         "failed to associate stdio stream with file descriptor %d, "
> +                         "file '%s' (mode: '%s')",

And I'd split this one like

                            "failed to associate stdio stream with"
                            " file descriptor %d, file '%s' (mode: '%s')",

> +                         fd, path, mode);
> +    }
>  
> -            close(fd);
> -            if (oflag & O_CREAT) {
> -                unlink(path);
> -            }
> +end:

Cases here:

1. find_open_flag() or open() failed (first two goto):
   @fd is -1, @f is NULL

2. open() succeeded, fchmod() failed (third goto), or
   open() succeeded, fdopen() failed (fall through):
   @fd is open, @f is NULL

3. open() and fdopen() succeeded:
   @fd and @f are open

> +    if (f == NULL && fd != -1) {

This is case 2.

> +        close(fd);
> +        if (oflag & O_CREAT) {
> +            unlink(path);
>          }
>      }
> -
> -    error_propagate(errp, local_err);
> -    return NULL;
> +    return f;

Works.

Case 1 could return NULL instead of goto end.  The condition for
close(fd) then becomes if (!f).  Feels simpler to me, but I guess it's a
matter of taste.

>  }
>  
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,

None of the above is serious, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create()
  2022-05-13 18:08 ` [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
@ 2022-05-16  7:24   ` Markus Armbruster
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2022-05-16  7:24 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel, Konstantin Kostiuk, Michael Roth

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function takes care of setting CLOEXEC, and reporting error.
>
> The reported error message will differ, from:
>   "failed to open file 'foo' (mode: 'r')"
> to:
>   "Failed to open file 'foo'"
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I don't like the first message's "(mode: 'r')", because it talks code to
the user.  Better: "for reading".  I'm not sure how useful the "for
reading" bit of information is here.  You decide.

Reviewed-by: Markus Armbruster <armbru@redhat.com>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 07/15] qga: throw an Error in ga_channel_open()
  2022-05-13 18:08 ` [PATCH v3 07/15] qga: throw an Error in ga_channel_open() marcandre.lureau
@ 2022-05-16  7:27   ` Markus Armbruster
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2022-05-16  7:27 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel, Konstantin Kostiuk, Michael Roth

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Allow for a single point of error reporting, and further refactoring.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/channel-posix.c | 43 ++++++++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index a996858e24..039e1ddcb2 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int fd)
>  }
>  
>  static gboolean ga_channel_open(GAChannel *c, const gchar *path,
> -                                GAChannelMethod method, int fd)
> +                                GAChannelMethod method, int fd, Error **errp)
>  {
> +    ERRP_GUARD();

I can't see where this is needed.

>      int ret;
>      c->method = method;
>  
> @@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>  #endif
>                             );
>          if (fd == -1) {
> -            g_critical("error opening channel: %s", strerror(errno));
> +            error_setg_errno(errp, errno, "error opening channel");
>              return false;
>          }
>  #ifdef CONFIG_SOLARIS
>          ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
>          if (ret == -1) {
> -            g_critical("error setting event mask for channel: %s",
> -                       strerror(errno));
> +            error_setg_errno(errp, errno, "error setting event mask for channel");
>              close(fd);
>              return false;
>          }
>  #endif
>          ret = ga_channel_client_add(c, fd);
>          if (ret) {
> -            g_critical("error adding channel to main loop");
> +            error_setg(errp, "error adding channel to main loop");
>              close(fd);
>              return false;
>          }
> @@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>          assert(fd < 0);
>          fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
>          if (fd == -1) {
> -            g_critical("error opening channel: %s", strerror(errno));
> +            error_setg_errno(errp, errno, "error opening channel");
>              return false;
>          }
>          tcgetattr(fd, &tio);
> @@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>          tcsetattr(fd, TCSANOW, &tio);
>          ret = ga_channel_client_add(c, fd);
>          if (ret) {
> -            g_critical("error adding channel to main loop");
> +            error_setg(errp, "error adding channel to main loop");
>              close(fd);
>              return false;
>          }
> @@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>      }
>      case GA_CHANNEL_UNIX_LISTEN: {
>          if (fd < 0) {
> -            Error *local_err = NULL;
> -
> -            fd = unix_listen(path, &local_err);
> -            if (local_err != NULL) {
> -                g_critical("%s", error_get_pretty(local_err));
> -                error_free(local_err);
> +            fd = unix_listen(path, errp);
> +            if (fd < 0) {
>                  return false;
>              }
>          }
> @@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>      }
>      case GA_CHANNEL_VSOCK_LISTEN: {
>          if (fd < 0) {
> -            Error *local_err = NULL;
>              SocketAddress *addr;
>              char *addr_str;
>  
>              addr_str = g_strdup_printf("vsock:%s", path);
> -            addr = socket_parse(addr_str, &local_err);
> +            addr = socket_parse(addr_str, errp);
>              g_free(addr_str);
> -            if (local_err != NULL) {
> -                g_critical("%s", error_get_pretty(local_err));
> -                error_free(local_err);
> +            if (!addr) {
>                  return false;
>              }
>  
> -            fd = socket_listen(addr, 1, &local_err);
> +            fd = socket_listen(addr, 1, errp);
>              qapi_free_SocketAddress(addr);
> -            if (local_err != NULL) {
> -                g_critical("%s", error_get_pretty(local_err));
> -                error_free(local_err);
> +            if (fd < 0) {
>                  return false;
>              }
>          }
> @@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
>          break;
>      }
>      default:
> -        g_critical("error binding/listening to specified socket");
> +        error_setg(errp, "error binding/listening to specified socket");
>          return false;
>      }
>  
> @@ -272,12 +263,14 @@ GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count)
>  GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
>                            int listen_fd, GAChannelCallback cb, gpointer opaque)
>  {
> +    Error *err = NULL;
>      GAChannel *c = g_new0(GAChannel, 1);
>      c->event_cb = cb;
>      c->user_data = opaque;
>  
> -    if (!ga_channel_open(c, path, method, listen_fd)) {
> -        g_critical("error opening channel");
> +    if (!ga_channel_open(c, path, method, listen_fd, &err)) {
> +        g_critical("%s", error_get_pretty(err));
> +        error_free(err);
>          ga_channel_free(c);
>          return NULL;
>      }



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 00/15] Misc cleanups
  2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
                   ` (14 preceding siblings ...)
  2022-05-13 18:08 ` [PATCH v3 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
@ 2022-05-19 17:05 ` Marc-André Lureau
  15 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2022-05-19 17:05 UTC (permalink / raw)
  To: qemu-devel
  Cc: Konstantin Kostiuk, Michael Roth, Markus Armbruster, Bonzini, Paolo

Hi

Before I send a v4 and hopefully final version, could somebody review
those patches:

- include: move qemu_*_exec_dir() to cutils
- osdep: export qemu_open_cloexec()

- qga: replace qemu_open_old() with qemu_open_cloexec()
- test/qga: use G_TEST_DIR to locate os-release test file

(Paolo sortof acked the v1, but not quite rigorously)

thanks!

On Fri, May 13, 2022 at 8:08 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> v3:
> - changed error_report_err() back to g_critical()
> - added "qga: make build_fs_mount_list() return a bool"
> - replaced g_clear_pointer() usage by open-coded version
> - dropped needless g_autoptr(GError) in tests
> - rebased, (dropped "include: adjust header guards after renaming")
> - some commit message rewording
> - added r-b tags
>
> v2:
> - drop "compiler.h: add QEMU_{BEGIN,END}_IGNORE_INITIALIZER_OVERRIDES",
>   "qobject/json-lexer: disable -Winitializer-overrides warnings" &
>   "qapi/error: add g_autoptr(Error) support" and adjust related code.
> - add "test/qga: use g_auto wherever sensible"
> - add r-b tags
>
> Marc-André Lureau (15):
>   include: move qemu_*_exec_dir() to cutils
>   util/win32: simplify qemu_get_local_state_dir()
>   tests: make libqmp buildable for win32
>   qga: flatten safe_open_or_create()
>   osdep: export qemu_open_cloexec()
>   qga: use qemu_open_cloexec() for safe_open_or_create()
>   qga: throw an Error in ga_channel_open()
>   qga: replace qemu_open_old() with qemu_open_cloexec()
>   qga: make build_fs_mount_list() return a bool
>   test/qga: use G_TEST_DIR to locate os-release test file
>   qga/wixl: prefer variables over environment
>   qga/wixl: require Mingw_bin
>   qga/wixl: simplify some pre-processing
>   qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
>   test/qga: use g_auto wherever sensible
>
>  configure                            |   9 +-
>  include/qemu/cutils.h                |   7 ++
>  include/qemu/osdep.h                 |   9 +-
>  meson.build                          |   5 +-
>  qemu-io.c                            |   1 +
>  qga/channel-posix.c                  |  55 +++++----
>  qga/commands-posix.c                 | 164 +++++++++++++--------------
>  qga/installer/qemu-ga.wxs            |  83 +++++---------
>  qga/meson.build                      |  11 +-
>  storage-daemon/qemu-storage-daemon.c |   1 +
>  tests/qtest/fuzz/fuzz.c              |   1 +
>  tests/qtest/libqmp.c                 |  34 +++++-
>  tests/qtest/libqmp.h                 |   2 +
>  tests/unit/test-qga.c                | 130 ++++++++-------------
>  util/cutils.c                        | 108 ++++++++++++++++++
>  util/osdep.c                         |  10 +-
>  util/oslib-posix.c                   |  81 -------------
>  util/oslib-win32.c                   |  53 +--------
>  18 files changed, 358 insertions(+), 406 deletions(-)
>
> --
> 2.36.1
>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec()
  2022-05-13 18:08 ` [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
@ 2022-05-20 13:39   ` Konstantin Kostiuk
  0 siblings, 0 replies; 31+ messages in thread
From: Konstantin Kostiuk @ 2022-05-20 13:39 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Michael Roth, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 5282 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Fri, May 13, 2022 at 9:08 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qemu_open_old() uses qemu_open_internal() which handles special
> "/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
> reporting (and some O_DIRECT special error casing).
>
> The monitor fdset handling is unnecessary for qga, use
> qemu_open_cloexec() instead.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/channel-posix.c  | 14 +++++++++-----
>  qga/commands-posix.c | 24 ++++++++++++------------
>  2 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 039e1ddcb2..affee485fc 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include <termios.h>
>  #include "qapi/error.h"
>  #include "qemu/sockets.h"
> @@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
>      switch (c->method) {
>      case GA_CHANNEL_VIRTIO_SERIAL: {
>          assert(fd < 0);
> -        fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
> +        fd = qemu_open_cloexec(
> +            path,
>  #ifndef CONFIG_SOLARIS
> -                           | O_ASYNC
> +            O_ASYNC |
>  #endif
> -                           );
> +            O_RDWR | O_NONBLOCK,
> +            0,
> +            errp
> +        );
>          if (fd == -1) {
>              error_setg_errno(errp, errno, "error opening channel");
>              return false;
> @@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
>          struct termios tio;
>
>          assert(fd < 0);
> -        fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> +        fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0,
> errp);
>          if (fd == -1) {
> -            error_setg_errno(errp, errno, "error opening channel");
>              return false;
>          }
>          tcgetattr(fd, &tio);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7761458ce1..e8eafad74d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1394,6 +1394,7 @@ static GuestDiskInfoList *get_disk_partitions(
>
>  static void get_nvme_smart(GuestDiskInfo *disk)
>  {
> +    Error *err = NULL;
>      int fd;
>      GuestNVMeSmart *smart;
>      NvmeSmartLog log = {0};
> @@ -1406,9 +1407,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
>                   | (((sizeof(log) >> 2) - 1) << 16)
>      };
>
> -    fd = qemu_open_old(disk->name, O_RDONLY);
> +    fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, &err);
>      if (fd == -1) {
> -        g_debug("Failed to open device: %s: %s", disk->name,
> g_strerror(errno));
> +        g_debug("Failed to open device: %s: %s", disk->name,
> error_get_pretty(err));
> +        error_free(err);
>          return;
>      }
>
> @@ -1739,9 +1741,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
> has_mountpoints,
>              }
>          }
>
> -        fd = qemu_open_old(mount->dirname, O_RDONLY);
> +        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
>          if (fd == -1) {
> -            error_setg_errno(errp, errno, "failed to open %s",
> mount->dirname);
>              goto error;
>          }
>
> @@ -1806,7 +1807,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>
>      QTAILQ_FOREACH(mount, &mounts, next) {
>          logged = false;
> -        fd = qemu_open_old(mount->dirname, O_RDONLY);
> +        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
>          if (fd == -1) {
>              continue;
>          }
> @@ -1866,21 +1867,20 @@ static void guest_fsfreeze_cleanup(void)
>  GuestFilesystemTrimResponse *
>  qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
> +    ERRP_GUARD();
>      GuestFilesystemTrimResponse *response;
>      GuestFilesystemTrimResult *result;
>      int ret = 0;
>      FsMountList mounts;
>      struct FsMount *mount;
>      int fd;
> -    Error *local_err = NULL;
>      struct fstrim_range r;
>
>      slog("guest-fstrim called");
>
>      QTAILQ_INIT(&mounts);
> -    build_fs_mount_list(&mounts, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    build_fs_mount_list(&mounts, errp);
> +    if (*errp) {
>          return NULL;
>      }
>
> @@ -1892,11 +1892,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t
> minimum, Error **errp)
>
>          QAPI_LIST_PREPEND(response->paths, result);
>
> -        fd = qemu_open_old(mount->dirname, O_RDONLY);
> +        fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
>          if (fd == -1) {
> -            result->error = g_strdup_printf("failed to open: %s",
> -                                            strerror(errno));
> +            result->error = g_strdup(error_get_pretty(*errp));
>              result->has_error = true;
> +            g_clear_pointer(errp, error_free);
>              continue;
>          }
>
> --
> 2.36.1
>
>

[-- Attachment #2: Type: text/html, Size: 6683 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file
  2022-05-13 18:08 ` [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
@ 2022-05-20 13:43   ` Konstantin Kostiuk
  0 siblings, 0 replies; 31+ messages in thread
From: Konstantin Kostiuk @ 2022-05-20 13:43 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Michael Roth, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Fri, May 13, 2022 at 9:08 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This a more accurate way to lookup the test data, and will allow to move
> the test in a subproject.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/unit/test-qga.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index d6df1ee92e..ab0b12a2dd 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -914,15 +914,14 @@ static void test_qga_guest_get_osinfo(gconstpointer
> data)
>  {
>      TestFixture fixture;
>      const gchar *str;
> -    gchar *cwd, *env[2];
> -    QDict *ret, *val;
> +    QDict *ret = NULL;
> +    char *env[2];

+    QDict *val;
>
> -    cwd = g_get_current_dir();
>      env[0] = g_strdup_printf(
> -        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
> -        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> +        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
> +        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR,
> G_DIR_SEPARATOR);
>      env[1] = NULL;
> -    g_free(cwd);
>      fixture_setup(&fixture, NULL, env);
>
>      ret = qmp_fd(fixture.fd, "{'execute': 'guest-get-osinfo'}");
> --
> 2.36.1
>
>

[-- Attachment #2: Type: text/html, Size: 2268 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
@ 2022-05-23 12:01   ` Markus Armbruster
  2022-05-23 12:29   ` Peter Maydell
  2022-05-23 12:42   ` Daniel P. Berrangé
  2 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2022-05-23 12:01 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel, Konstantin Kostiuk, Michael Roth

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Used in the next patch, to simplify qga code.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

The commit mesage makes me expect a change to external linkage plus a
declaration in the header, but ...

> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..545a88e1fd 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>  }
>  #endif
>  
> -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
>  {
> +    ERRP_GUARD();

Looks redundant to me.

>      int ret;
> +
>  #ifdef O_CLOEXEC
>      ret = open(name, flags | O_CLOEXEC, mode);
>  #else
> @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>          qemu_set_cloexec(ret);
>      }
>  #endif
> +    if (ret == -1) {
> +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> +    }
> +
>      return ret;
>  }
>  
> @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>      }
>  #endif
>  
> -    ret = qemu_open_cloexec(name, flags, mode);
> +    ret = qemu_open_cloexec(name, flags, mode, NULL);
>  
>      if (ret == -1) {
>          const char *action = flags & O_CREAT ? "create" : "open";

... the patch additionally changes how errors are communicated.  Please
cover that in the commit message.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils
  2022-05-13 18:08 ` [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
@ 2022-05-23 12:23   ` Markus Armbruster
  0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2022-05-23 12:23 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel, Konstantin Kostiuk, Michael Roth

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function is required by get_relocated_path() (already in cutils),
> and used by qemu-ga and may be generally useful.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/cutils.h                |   7 ++
>  include/qemu/osdep.h                 |   8 --
>  qemu-io.c                            |   1 +
>  storage-daemon/qemu-storage-daemon.c |   1 +
>  tests/qtest/fuzz/fuzz.c              |   1 +
>  util/cutils.c                        | 108 +++++++++++++++++++++++++++
>  util/oslib-posix.c                   |  81 --------------------
>  util/oslib-win32.c                   |  36 ---------
>  8 files changed, 118 insertions(+), 125 deletions(-)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 5c6572d444..40e10e19a7 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -193,6 +193,13 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>   */
>  int qemu_pstrcmp0(const char **str1, const char **str2);
>  
> +/* Find program directory, and save it for later usage with
> + * qemu_get_exec_dir().
> + * Try OS specific API first, if not working, parse from argv0. */
> +void qemu_init_exec_dir(const char *argv0);
> +
> +/* Get the saved exec dir.  */
> +const char *qemu_get_exec_dir(void);
>  
>  /**
>   * get_relocated_path:
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 1c1e7eca98..67cc465416 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -557,14 +557,6 @@ void qemu_set_cloexec(int fd);
>   */
>  char *qemu_get_local_state_dir(void);
>  
> -/* Find program directory, and save it for later usage with
> - * qemu_get_exec_dir().
> - * Try OS specific API first, if not working, parse from argv0. */
> -void qemu_init_exec_dir(const char *argv0);
> -
> -/* Get the saved exec dir.  */
> -const char *qemu_get_exec_dir(void);
> -
>  /**
>   * qemu_getauxval:
>   * @type: the auxiliary vector key to lookup
> diff --git a/qemu-io.c b/qemu-io.c
> index d70d3dd4fd..2bd7bfb650 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -16,6 +16,7 @@
>  #endif
>  
>  #include "qemu/help-texts.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "qemu-io.h"
>  #include "qemu/error-report.h"
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index 9b8b17f52e..c104817cdd 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -44,6 +44,7 @@
>  
>  #include "qemu/help-texts.h"
>  #include "qemu-version.h"
> +#include "qemu/cutils.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
>  #include "qemu/help_option.h"
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index a7a5e14fa3..0ad4ba9e94 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -15,6 +15,7 @@
>  
>  #include <wordexp.h>
>  
> +#include "qemu/cutils.h"
>  #include "qemu/datadir.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/qtest.h"
> diff --git a/util/cutils.c b/util/cutils.c
> index b2777210e7..6cc7cc8cde 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -931,6 +931,114 @@ static inline const char *next_component(const char *dir, int *p_len)
>      return dir;
>  }
>  
> +static const char *exec_dir;
> +
> +void qemu_init_exec_dir(const char *argv0)
> +{
> +#ifdef G_OS_WIN32
> +    char *p;
> +    char buf[MAX_PATH];
> +    DWORD len;
> +
> +    if (exec_dir) {
> +        return;
> +    }
> +
> +    len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
> +    if (len == 0) {
> +        return;
> +    }
> +
> +    buf[len] = 0;
> +    p = buf + len - 1;
> +    while (p != buf && *p != '\\') {
> +        p--;
> +    }
> +    *p = 0;
> +    if (access(buf, R_OK) == 0) {
> +        exec_dir = g_strdup(buf);
> +    } else {
> +        exec_dir = CONFIG_BINDIR;
> +    }
> +#else
> +    char *p = NULL;
> +    char buf[PATH_MAX];
> +
> +    if (exec_dir) {
> +        return;
> +    }
> +
> +#if defined(__linux__)
> +    {
> +        int len;
> +        len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
> +        if (len > 0) {
> +            buf[len] = 0;
> +            p = buf;
> +        }
> +    }
> +#elif defined(__FreeBSD__) \
> +      || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
> +    {
> +#if defined(__FreeBSD__)
> +        static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
> +#else
> +        static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};
> +#endif
> +        size_t len = sizeof(buf) - 1;
> +
> +        *buf = '\0';
> +        if (!sysctl(mib, ARRAY_SIZE(mib), buf, &len, NULL, 0) &&
> +            *buf) {
> +            buf[sizeof(buf) - 1] = '\0';
> +            p = buf;
> +        }
> +    }
> +#elif defined(__APPLE__)
> +    {
> +        char fpath[PATH_MAX];
> +        uint32_t len = sizeof(fpath);
> +        if (_NSGetExecutablePath(fpath, &len) == 0) {
> +            p = realpath(fpath, buf);
> +            if (!p) {
> +                return;
> +            }
> +        }
> +    }
> +#elif defined(__HAIKU__)
> +    {
> +        image_info ii;
> +        int32_t c = 0;
> +
> +        *buf = '\0';
> +        while (get_next_image_info(0, &c, &ii) == B_OK) {
> +            if (ii.type == B_APP_IMAGE) {
> +                strncpy(buf, ii.name, sizeof(buf));
> +                buf[sizeof(buf) - 1] = 0;
> +                p = buf;
> +                break;
> +            }
> +        }
> +    }
> +#endif
> +    /* If we don't have any way of figuring out the actual executable
> +       location then try argv[0].  */
> +    if (!p && argv0) {
> +        p = realpath(argv0, buf);
> +    }
> +    if (p) {
> +        exec_dir = g_path_get_dirname(p);
> +    } else {
> +        exec_dir = CONFIG_BINDIR;
> +    }
> +#endif
> +}

Combines code moved from oslib-posix.c and oslib-win32.c with #if.
Sounds like a step backward, until you realize just how many #if there
already are.  Okay.

> +
> +const char *qemu_get_exec_dir(void)
> +{
> +    return exec_dir;
> +}
> +
>  char *get_relocated_path(const char *dir)
>  {
>      size_t prefix_len = strlen(CONFIG_PREFIX);

Keeping qemu_init_exec_dir() and qemu_get_exec_dir() next to
get_relocated_path() makes sense.  However, util/cutils.c is getting
rather long: almost 700 SLOC, and I feel the new code stretches its
"Simple C functions to supplement the C library" mandate.  Most of the
code there is string stuff.  Would you mind putting the file name stuff
into its own file?

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
  2022-05-23 12:01   ` Markus Armbruster
@ 2022-05-23 12:29   ` Peter Maydell
  2022-05-23 12:42   ` Daniel P. Berrangé
  2 siblings, 0 replies; 31+ messages in thread
From: Peter Maydell @ 2022-05-23 12:29 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: qemu-devel, Konstantin Kostiuk, Michael Roth, Markus Armbruster

On Fri, 13 May 2022 at 19:16, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Used in the next patch, to simplify qga code.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);

Where we make a function global and put it in a header
we should generally add a documentation comment describing it,
please.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
  2022-05-23 12:01   ` Markus Armbruster
  2022-05-23 12:29   ` Peter Maydell
@ 2022-05-23 12:42   ` Daniel P. Berrangé
  2022-05-23 17:30     ` Marc-André Lureau
  2 siblings, 1 reply; 31+ messages in thread
From: Daniel P. Berrangé @ 2022-05-23 12:42 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: qemu-devel, Konstantin Kostiuk, Michael Roth, Markus Armbruster

On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Used in the next patch, to simplify qga code.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);

I don't think we should be exporting this - it is just a variant of the
'qemu_open_old' method that we wanted callers to stop using in favour
of explicitly deciding between 'qemu_open' and 'qemu_create'.

>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..545a88e1fd 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>  }
>  #endif
>  
> -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
>  {
> +    ERRP_GUARD();
>      int ret;
> +
>  #ifdef O_CLOEXEC
>      ret = open(name, flags | O_CLOEXEC, mode);
>  #else
> @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>          qemu_set_cloexec(ret);
>      }
>  #endif
> +    if (ret == -1) {
> +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> +    }
> +

This will mean that qemu_open_internal() caller will now be overwriting
an existing error message.

>      return ret;
>  }
>  
> @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>      }
>  #endif
>  
> -    ret = qemu_open_cloexec(name, flags, mode);
> +    ret = qemu_open_cloexec(name, flags, mode, NULL);
>  
>      if (ret == -1) {
>          const char *action = flags & O_CREAT ? "create" : "open";
> -- 
> 2.36.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-23 12:42   ` Daniel P. Berrangé
@ 2022-05-23 17:30     ` Marc-André Lureau
  2022-05-23 17:56       ` Daniel P. Berrangé
  0 siblings, 1 reply; 31+ messages in thread
From: Marc-André Lureau @ 2022-05-23 17:30 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: QEMU, Konstantin Kostiuk, Michael Roth, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

Hi

On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Used in the next patch, to simplify qga code.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qemu/osdep.h |  1 +
> >  util/osdep.c         | 10 ++++++++--
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 67cc465416..64f51cfb7a 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> >   */
> >  int qemu_open_old(const char *name, int flags, ...);
> >  int qemu_open(const char *name, int flags, Error **errp);
> > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> **errp);
>
> I don't think we should be exporting this - it is just a variant of the
> 'qemu_open_old' method that we wanted callers to stop using in favour
> of explicitly deciding between 'qemu_open' and 'qemu_create'.
>


qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools don't
need.

(qemu_open_old() doesn't set CLOEXEC)


>
> >  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
> >  int qemu_close(int fd);
> >  int qemu_unlink(const char *name);
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 60fcbbaebe..545a88e1fd 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start,
> int64_t len, bool exclusive)
> >  }
> >  #endif
> >
> > -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> > +
> >  #ifdef O_CLOEXEC
> >      ret = open(name, flags | O_CLOEXEC, mode);
> >  #else
> > @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int
> flags, mode_t mode)
> >          qemu_set_cloexec(ret);
> >      }
> >  #endif
> > +    if (ret == -1) {
> > +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> > +    }
> > +
>
> This will mean that qemu_open_internal() caller will now be overwriting
> an existing error message.
>

NULL is passed as errp argument of qemu_open_cloexec() in
qemu_open_internal().


>
> >      return ret;
> >  }
> >
> > @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags,
> mode_t mode, Error **errp)
> >      }
> >  #endif
> >
> > -    ret = qemu_open_cloexec(name, flags, mode);
> > +    ret = qemu_open_cloexec(name, flags, mode, NULL);
> >
>

here


> >      if (ret == -1) {
> >          const char *action = flags & O_CREAT ? "create" : "open";
> > --
> > 2.36.1
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5383 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-23 17:30     ` Marc-André Lureau
@ 2022-05-23 17:56       ` Daniel P. Berrangé
  2022-05-23 18:02         ` Marc-André Lureau
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel P. Berrangé @ 2022-05-23 17:56 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: QEMU, Konstantin Kostiuk, Michael Roth, Markus Armbruster

On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Used in the next patch, to simplify qga code.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  include/qemu/osdep.h |  1 +
> > >  util/osdep.c         | 10 ++++++++--
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 67cc465416..64f51cfb7a 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > >   */
> > >  int qemu_open_old(const char *name, int flags, ...);
> > >  int qemu_open(const char *name, int flags, Error **errp);
> > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> > **errp);
> >
> > I don't think we should be exporting this - it is just a variant of the
> > 'qemu_open_old' method that we wanted callers to stop using in favour
> > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> >
> 
> 
> qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools don't
> need.

Right, but exporting this as 'qemu_open_cloexec' is going to mislead
people into thinking it is a better version of 'qemu_open'. This will
cause us to loose support for /dev/fdset in places where we actually
need it.

It is pretty harmless to have /dev/fdset there, even if the tool does
not need it - that's been the case with many QEMU tools for many years.
If we think it is actually a real problem though, we should just have
a way to toggle it on/off from the existing APIs.

eg put  'bool allow_fdset = true"   in softmmu/vl.c, and
'bool allow_fdset = false' in stubs/open.c, and then make
qemu_open_internal conditionalize itself on this global 
variable, so only the system emulators get fdset support
activated.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-23 17:56       ` Daniel P. Berrangé
@ 2022-05-23 18:02         ` Marc-André Lureau
  2022-05-23 18:10           ` Daniel P. Berrangé
  0 siblings, 1 reply; 31+ messages in thread
From: Marc-André Lureau @ 2022-05-23 18:02 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: QEMU, Konstantin Kostiuk, Michael Roth, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]

Hi

On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > Used in the next patch, to simplify qga code.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  include/qemu/osdep.h |  1 +
> > > >  util/osdep.c         | 10 ++++++++--
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > index 67cc465416..64f51cfb7a 100644
> > > > --- a/include/qemu/osdep.h
> > > > +++ b/include/qemu/osdep.h
> > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > > >   */
> > > >  int qemu_open_old(const char *name, int flags, ...);
> > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> Error
> > > **errp);
> > >
> > > I don't think we should be exporting this - it is just a variant of the
> > > 'qemu_open_old' method that we wanted callers to stop using in favour
> > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > >
> >
> >
> > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> don't
> > need.
>
> Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> people into thinking it is a better version of 'qemu_open'. This will
> cause us to loose support for /dev/fdset in places where we actually
> need it.
>

> It is pretty harmless to have /dev/fdset there, even if the tool does
> not need it - that's been the case with many QEMU tools for many years.
> If we think it is actually a real problem though, we should just have
> a way to toggle it on/off from the existing APIs.
>
>
It's a bit problematic to make qemu-ga standalone, and have a common shared
subproject/library.

Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any better
idea ?

eg put  'bool allow_fdset = true"   in softmmu/vl.c, and
> 'bool allow_fdset = false' in stubs/open.c, and then make
> qemu_open_internal conditionalize itself on this global
> variable, so only the system emulators get fdset support
> activated.
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4776 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-23 18:02         ` Marc-André Lureau
@ 2022-05-23 18:10           ` Daniel P. Berrangé
  2022-05-23 19:11             ` Marc-André Lureau
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel P. Berrangé @ 2022-05-23 18:10 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: QEMU, Konstantin Kostiuk, Michael Roth, Markus Armbruster

On Mon, May 23, 2022 at 08:02:45PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > > > wrote:
> > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > >
> > > > > Used in the next patch, to simplify qga code.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > >  include/qemu/osdep.h |  1 +
> > > > >  util/osdep.c         | 10 ++++++++--
> > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > index 67cc465416..64f51cfb7a 100644
> > > > > --- a/include/qemu/osdep.h
> > > > > +++ b/include/qemu/osdep.h
> > > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > > > >   */
> > > > >  int qemu_open_old(const char *name, int flags, ...);
> > > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> > Error
> > > > **errp);
> > > >
> > > > I don't think we should be exporting this - it is just a variant of the
> > > > 'qemu_open_old' method that we wanted callers to stop using in favour
> > > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > > >
> > >
> > >
> > > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> > don't
> > > need.
> >
> > Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> > people into thinking it is a better version of 'qemu_open'. This will
> > cause us to loose support for /dev/fdset in places where we actually
> > need it.
> >
> 
> > It is pretty harmless to have /dev/fdset there, even if the tool does
> > not need it - that's been the case with many QEMU tools for many years.
> > If we think it is actually a real problem though, we should just have
> > a way to toggle it on/off from the existing APIs.
> >
> >
> It's a bit problematic to make qemu-ga standalone, and have a common shared
> subproject/library.
> 
> Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any better
> idea ?

If we want to make qemu-ga standalone, then IMHO we should be
aggressively switching it to use as many GLib APIs as possible,
eliminating its reliance on any of QEMU's home-grown portability
functions. All the 'FILE *' / 'open' scenarios could be replaced
with GIO's GFile/GInputStream/GOutputStream for example.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
  2022-05-23 18:10           ` Daniel P. Berrangé
@ 2022-05-23 19:11             ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2022-05-23 19:11 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: QEMU, Konstantin Kostiuk, Michael Roth, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]

Hi

On Mon, May 23, 2022 at 8:11 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, May 23, 2022 at 08:02:45PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <
> berrange@redhat.com>
> > > > wrote:
> > > >
> > > > > On Fri, May 13, 2022 at 08:08:11PM +0200,
> marcandre.lureau@redhat.com
> > > > > wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > >
> > > > > > Used in the next patch, to simplify qga code.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >  include/qemu/osdep.h |  1 +
> > > > > >  util/osdep.c         | 10 ++++++++--
> > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > > index 67cc465416..64f51cfb7a 100644
> > > > > > --- a/include/qemu/osdep.h
> > > > > > +++ b/include/qemu/osdep.h
> > > > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction
> *action,
> > > > > >   */
> > > > > >  int qemu_open_old(const char *name, int flags, ...);
> > > > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> > > Error
> > > > > **errp);
> > > > >
> > > > > I don't think we should be exporting this - it is just a variant
> of the
> > > > > 'qemu_open_old' method that we wanted callers to stop using in
> favour
> > > > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > > > >
> > > >
> > > >
> > > > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> > > don't
> > > > need.
> > >
> > > Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> > > people into thinking it is a better version of 'qemu_open'. This will
> > > cause us to loose support for /dev/fdset in places where we actually
> > > need it.
> > >
> >
> > > It is pretty harmless to have /dev/fdset there, even if the tool does
> > > not need it - that's been the case with many QEMU tools for many years.
> > > If we think it is actually a real problem though, we should just have
> > > a way to toggle it on/off from the existing APIs.
> > >
> > >
> > It's a bit problematic to make qemu-ga standalone, and have a common
> shared
> > subproject/library.
> >
> > Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any
> better
> > idea ?
>
> If we want to make qemu-ga standalone, then IMHO we should be
> aggressively switching it to use as many GLib APIs as possible,
> eliminating its reliance on any of QEMU's home-grown portability
> functions. All the 'FILE *' / 'open' scenarios could be replaced
> with GIO's GFile/GInputStream/GOutputStream for example.
>

I am not too eager to do that kind of refactoring. Even rewriting in Rust
seems a bit pointless to me, even if I would have more motivation.

Also there are times you do open() for things that are not stream-related.
And glib sadly doesn't really offer a solution for open(CLOEXEC).

I guess I can simply add an open_cloexec() helper function in qemu-ga alone
for now.

[-- Attachment #2: Type: text/html, Size: 4946 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2022-05-23 19:13 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13 18:08 [PATCH v3 00/15] Misc cleanups marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
2022-05-23 12:23   ` Markus Armbruster
2022-05-13 18:08 ` [PATCH v3 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 03/15] tests: make libqmp buildable for win32 marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 04/15] qga: flatten safe_open_or_create() marcandre.lureau
2022-05-16  7:19   ` Markus Armbruster
2022-05-13 18:08 ` [PATCH v3 05/15] osdep: export qemu_open_cloexec() marcandre.lureau
2022-05-23 12:01   ` Markus Armbruster
2022-05-23 12:29   ` Peter Maydell
2022-05-23 12:42   ` Daniel P. Berrangé
2022-05-23 17:30     ` Marc-André Lureau
2022-05-23 17:56       ` Daniel P. Berrangé
2022-05-23 18:02         ` Marc-André Lureau
2022-05-23 18:10           ` Daniel P. Berrangé
2022-05-23 19:11             ` Marc-André Lureau
2022-05-13 18:08 ` [PATCH v3 06/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
2022-05-16  7:24   ` Markus Armbruster
2022-05-13 18:08 ` [PATCH v3 07/15] qga: throw an Error in ga_channel_open() marcandre.lureau
2022-05-16  7:27   ` Markus Armbruster
2022-05-13 18:08 ` [PATCH v3 08/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
2022-05-20 13:39   ` Konstantin Kostiuk
2022-05-13 18:08 ` [PATCH v3 09/15] qga: make build_fs_mount_list() return a bool marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
2022-05-20 13:43   ` Konstantin Kostiuk
2022-05-13 18:08 ` [PATCH v3 11/15] qga/wixl: prefer variables over environment marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 12/15] qga/wixl: require Mingw_bin marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
2022-05-13 18:08 ` [PATCH v3 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
2022-05-19 17:05 ` [PATCH v3 00/15] Misc cleanups Marc-André Lureau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.