* [PATCH v2 00/15] Misc cleanups
@ 2022-05-05 8:14 marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
` (14 more replies)
0 siblings, 15 replies; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Hi,
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
include: adjust header guards after renaming
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()
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 +-
meson.build | 5 +-
include/qemu/cutils.h | 7 ++
include/qemu/help-texts.h | 4 +-
include/qemu/osdep.h | 9 +-
tests/qtest/libqmp.h | 2 +
qemu-io.c | 1 +
qga/channel-posix.c | 54 +++++-----
qga/commands-posix.c | 145 +++++++++++++--------------
storage-daemon/qemu-storage-daemon.c | 1 +
tests/qtest/fuzz/fuzz.c | 1 +
tests/qtest/libqmp.c | 35 +++++--
tests/unit/test-qga.c | 134 +++++++++----------------
util/cutils.c | 108 ++++++++++++++++++++
util/osdep.c | 10 +-
util/oslib-posix.c | 81 ---------------
util/oslib-win32.c | 53 +---------
qga/installer/qemu-ga.wxs | 83 ++++++---------
qga/meson.build | 11 +-
19 files changed, 352 insertions(+), 401 deletions(-)
--
2.36.0.44.g0f828332d5ac
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
` (13 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 5c6572d44422..40e10e19a7ed 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 1c1e7eca9898..67cc4654166b 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 d70d3dd4fde5..2bd7bfb65073 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 9b8b17f52e48..c104817cdddc 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 a7a5e14fa3bc..0ad4ba9e94dc 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 b2777210e7da..6cc7cc8cde99 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 477990f39baf..7ba4472760ba 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 dafef4f15733..6c818749d2b9 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 03/15] tests: make libqmp buildable for win32 marcandre.lureau
` (12 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 6c818749d2b9..5723d3eb4c5a 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 03/15] tests: make libqmp buildable for win32
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 10:52 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 04/15] include: adjust header guards after renaming marcandre.lureau
` (11 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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.h | 2 ++
tests/qtest/libqmp.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
index 94aa97328a17..772f18b73ba3 100644
--- a/tests/qtest/libqmp.h
+++ b/tests/qtest/libqmp.h
@@ -20,8 +20,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);
diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
index 0358b8313dc4..93c9b31cd4ca 100644
--- a/tests/qtest/libqmp.c
+++ b/tests/qtest/libqmp.c
@@ -15,9 +15,13 @@
*/
#include "qemu/osdep.h"
-
#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 +91,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 +125,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 +159,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 +175,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);
}
--
2.36.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 04/15] include: adjust header guards after renaming
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (2 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 03/15] tests: make libqmp buildable for win32 marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 10:58 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 05/15] qga: flatten safe_open_or_create() marcandre.lureau
` (10 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
---
include/qemu/help-texts.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qemu/help-texts.h b/include/qemu/help-texts.h
index ba32cc8b1f39..4f265fed8df1 100644
--- a/include/qemu/help-texts.h
+++ b/include/qemu/help-texts.h
@@ -1,5 +1,5 @@
-#ifndef QEMU_COMMON_H
-#define QEMU_COMMON_H
+#ifndef QEMU_HELP_TEXTS_H
+#define QEMU_HELP_TEXTS_H
/* Copyright string for -version arguments, About dialogs, etc */
#define QEMU_COPYRIGHT "Copyright (c) 2003-2022 " \
--
2.36.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 05/15] qga: flatten safe_open_or_create()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (3 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 04/15] include: adjust header guards after renaming marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 11:19 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 06/15] osdep: export qemu_open_cloexec() marcandre.lureau
` (9 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
There is a bit too much branching in the function, this can be
simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
This also helps with the following error handling changes.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qga/commands-posix.c | 124 ++++++++++++++++++++++---------------------
1 file changed, 63 insertions(+), 61 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 69f209af87e6..0ef049650e31 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;
-
- oflag = find_open_flag(mode, &local_err);
- if (local_err == NULL) {
- int fd;
+ ERRP_GUARD();
+ int oflag, fd = -1;
+ FILE *f = NULL;
+
+ oflag = find_open_flag(mode, errp);
+ if (*errp) {
+ 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;
+ }
- /* 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 06/15] osdep: export qemu_open_cloexec()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (4 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 05/15] qga: flatten safe_open_or_create() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
` (8 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 67cc4654166b..64f51cfb7a62 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 60fcbbaebe72..67541b7654ef 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, "Could not open '%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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (5 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 06/15] osdep: export qemu_open_cloexec() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 11:32 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 08/15] qga: throw an Error in ga_channel_open() marcandre.lureau
` (7 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
The function takes care of setting CLOEXEC, and reporting error.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qga/commands-posix.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ef049650e31..8ebc327c5e02 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -370,21 +370,16 @@ 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) {
+ g_clear_pointer(errp, error_free);
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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 08/15] qga: throw an Error in ga_channel_open()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (6 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 11:39 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
` (6 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 | 42 +++++++++++++++++-------------------------
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index a996858e2492..0ce594bc36c2 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 (*errp) {
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 (*errp) {
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,13 @@ 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)) {
+ error_report_err(err);
ga_channel_free(c);
return NULL;
}
--
2.36.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (7 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 08/15] qga: throw an Error in ga_channel_open() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 11:42 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
` (5 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 0ce594bc36c2..a1262b130145 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 8ebc327c5e02..f82205e25813 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
static void get_nvme_smart(GuestDiskInfo *disk)
{
+ Error *err = NULL;
int fd;
GuestNVMeSmart *smart;
NvmeSmartLog log = {0};
@@ -1404,9 +1405,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;
}
@@ -1737,9 +1739,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;
}
@@ -1804,7 +1805,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;
}
@@ -1864,21 +1865,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;
}
@@ -1890,11 +1890,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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 10/15] test/qga: use G_TEST_DIR to locate os-release test file
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (8 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 11/15] qga/wixl: prefer variables over environment marcandre.lureau
` (4 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 d6df1ee92ea1..ab0b12a2dd16 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 11/15] qga/wixl: prefer variables over environment
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (9 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:25 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 12/15] qga/wixl: require Mingw_bin marcandre.lureau
` (3 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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>
---
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 0950e8c6becc..8a19aa165651 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 6d9f39bb321b..3ad3bc0260cf 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 12/15] qga/wixl: require Mingw_bin
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (10 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 11/15] qga/wixl: prefer variables over environment marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:26 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
` (2 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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>
---
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 8a19aa165651..651db6e51cda 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 13/15] qga/wixl: simplify some pre-processing
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (11 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 12/15] qga/wixl: require Mingw_bin marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:26 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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>
---
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 651db6e51cda..e5b0958e1898 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (12 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 8:28 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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>
---
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 59c43bea05eb..616cd2d0e36c 100755
--- a/configure
+++ b/configure
@@ -2023,6 +2023,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
@@ -2430,8 +2435,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"; } && \
@@ -2518,7 +2521,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
@@ -2639,6 +2641,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 c26aa442d40e..2f68b6cb8634 100644
--- a/meson.build
+++ b/meson.build
@@ -443,7 +443,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 e5b0958e1898..813d1c6ca6ae 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 3ad3bc0260cf..51b1e611b194 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 15/15] test/qga: use g_auto wherever sensible
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
` (13 preceding siblings ...)
2022-05-05 8:14 ` [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
@ 2022-05-05 8:14 ` marcandre.lureau
2022-05-05 11:47 ` Markus Armbruster
14 siblings, 1 reply; 35+ messages in thread
From: marcandre.lureau @ 2022-05-05 8:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block, 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 | 125 +++++++++++++++---------------------------
1 file changed, 45 insertions(+), 80 deletions(-)
diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index ab0b12a2dd16..53cefc2c2649 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -51,8 +51,11 @@ static void
fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
{
const gchar *extra_arg = data;
- GError *error = NULL;
- gchar *cwd, *path, *cmd, **argv = NULL;
+ g_autoptr(GError) error = 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)
@@ -692,19 +666,23 @@ 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_autoptr(GError) error = 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.0.44.g0f828332d5ac
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 11/15] qga/wixl: prefer variables over environment
2022-05-05 8:14 ` [PATCH v2 11/15] qga/wixl: prefer variables over environment marcandre.lureau
@ 2022-05-05 8:25 ` Konstantin Kostiuk
0 siblings, 0 replies; 35+ messages in thread
From: Konstantin Kostiuk @ 2022-05-05 8:25 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 6414 bytes --]
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
On Thu, May 5, 2022 at 11:16 AM <marcandre.lureau@redhat.com> wrote:
> 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>
> ---
> 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 0950e8c6becc..8a19aa165651 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 6d9f39bb321b..3ad3bc0260cf 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.0.44.g0f828332d5ac
>
>
[-- Attachment #2: Type: text/html, Size: 9169 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 12/15] qga/wixl: require Mingw_bin
2022-05-05 8:14 ` [PATCH v2 12/15] qga/wixl: require Mingw_bin marcandre.lureau
@ 2022-05-05 8:26 ` Konstantin Kostiuk
0 siblings, 0 replies; 35+ messages in thread
From: Konstantin Kostiuk @ 2022-05-05 8:26 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
On Thu, May 5, 2022 at 11:16 AM <marcandre.lureau@redhat.com> wrote:
> 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>
> ---
> 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 8a19aa165651..651db6e51cda 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.0.44.g0f828332d5ac
>
>
[-- Attachment #2: Type: text/html, Size: 1843 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 13/15] qga/wixl: simplify some pre-processing
2022-05-05 8:14 ` [PATCH v2 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
@ 2022-05-05 8:26 ` Konstantin Kostiuk
0 siblings, 0 replies; 35+ messages in thread
From: Konstantin Kostiuk @ 2022-05-05 8:26 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
On Thu, May 5, 2022 at 11:16 AM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Sadly, wixl doesn't have 'elif'.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 651db6e51cda..e5b0958e1898 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.0.44.g0f828332d5ac
>
>
[-- Attachment #2: Type: text/html, Size: 2497 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
2022-05-05 8:14 ` [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
@ 2022-05-05 8:28 ` Konstantin Kostiuk
0 siblings, 0 replies; 35+ messages in thread
From: Konstantin Kostiuk @ 2022-05-05 8:28 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Markus Armbruster, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 10033 bytes --]
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
On Thu, May 5, 2022 at 11:16 AM <marcandre.lureau@redhat.com> wrote:
> 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>
> ---
> 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 59c43bea05eb..616cd2d0e36c 100755
> --- a/configure
> +++ b/configure
> @@ -2023,6 +2023,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
>
> @@ -2430,8 +2435,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"; } && \
> @@ -2518,7 +2521,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
> @@ -2639,6 +2641,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 c26aa442d40e..2f68b6cb8634 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -443,7 +443,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 e5b0958e1898..813d1c6ca6ae 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 3ad3bc0260cf..51b1e611b194 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.0.44.g0f828332d5ac
>
>
[-- Attachment #2: Type: text/html, Size: 13773 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 03/15] tests: make libqmp buildable for win32
2022-05-05 8:14 ` [PATCH v2 03/15] tests: make libqmp buildable for win32 marcandre.lureau
@ 2022-05-05 10:52 ` Markus Armbruster
2022-05-05 11:08 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 10:52 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> 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.h | 2 ++
> tests/qtest/libqmp.c | 35 +++++++++++++++++++++++++++++------
> 2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
> index 94aa97328a17..772f18b73ba3 100644
> --- a/tests/qtest/libqmp.h
> +++ b/tests/qtest/libqmp.h
> @@ -20,8 +20,10 @@
> #include "qapi/qmp/qdict.h"
>
> QDict *qmp_fd_receive(int fd);
> +#ifndef G_OS_WIN32
What's the difference between G_OS_WIN32 and _WIN32?
We have 10 of the former, but >250 of the latter. If they are
effectively the same, we should pick one and stick to it.
[...]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 04/15] include: adjust header guards after renaming
2022-05-05 8:14 ` [PATCH v2 04/15] include: adjust header guards after renaming marcandre.lureau
@ 2022-05-05 10:58 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 10:58 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
Pointing to commit 49f9522193 "include: rename qemu-common.h
qemu/help-texts.h" wouldn't hurt.
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> ---
> include/qemu/help-texts.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/help-texts.h b/include/qemu/help-texts.h
> index ba32cc8b1f39..4f265fed8df1 100644
> --- a/include/qemu/help-texts.h
> +++ b/include/qemu/help-texts.h
> @@ -1,5 +1,5 @@
> -#ifndef QEMU_COMMON_H
> -#define QEMU_COMMON_H
> +#ifndef QEMU_HELP_TEXTS_H
> +#define QEMU_HELP_TEXTS_H
>
> /* Copyright string for -version arguments, About dialogs, etc */
> #define QEMU_COPYRIGHT "Copyright (c) 2003-2022 " \
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Time for a re-run of scripts/clean-header-guards.pl.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 03/15] tests: make libqmp buildable for win32
2022-05-05 10:52 ` Markus Armbruster
@ 2022-05-05 11:08 ` Marc-André Lureau
2022-05-16 5:57 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-05 11:08 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
Hi
On Thu, May 5, 2022 at 2:52 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > 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.h | 2 ++
> > tests/qtest/libqmp.c | 35 +++++++++++++++++++++++++++++------
> > 2 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
> > index 94aa97328a17..772f18b73ba3 100644
> > --- a/tests/qtest/libqmp.h
> > +++ b/tests/qtest/libqmp.h
> > @@ -20,8 +20,10 @@
> > #include "qapi/qmp/qdict.h"
> >
> > QDict *qmp_fd_receive(int fd);
> > +#ifndef G_OS_WIN32
>
> What's the difference between G_OS_WIN32 and _WIN32?
>
> We have 10 of the former, but >250 of the latter. If they are
> effectively the same, we should pick one and stick to it.
There are some subtle differences when compiling for cygwin, in which
case G_OS_WIN32 is not defined.
I usually pick G_OS_{UNIX,WIN32} defines, mostly for consistency, but
in many situation _WIN32/WIN32 is fine.
(and we also have CONFIG_WIN32)
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()
2022-05-05 8:14 ` [PATCH v2 05/15] qga: flatten safe_open_or_create() marcandre.lureau
@ 2022-05-05 11:19 ` Markus Armbruster
2022-05-05 11:27 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 11:19 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> There is a bit too much branching in the function, this can be
> simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
Do you mean ERRP_GUARD()?
I'm not sure the commit reduces "branching", but it certainly reduces
nesting, which I find improves readability.
> This also helps with the following error handling changes.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/commands-posix.c | 124 ++++++++++++++++++++++---------------------
> 1 file changed, 63 insertions(+), 61 deletions(-)
I think the diff is easier to review with space change ignored:
| diff --git a/qga/commands-posix.c b/qga/commands-posix.c
| index 78f2f21001..b4b19d3eb4 100644
| --- a/qga/commands-posix.c
| +++ b/qga/commands-posix.c
| @@ -315,12 +315,14 @@ 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;
| + ERRP_GUARD();
| + int oflag, fd = -1;
I'd prefer
+ ERRP_GUARD();
int oflag;
+ int fd = -1;
because it's slightly less churn, and I dislike variables with and
without initializer in the same declaration. Matter of taste.
| + FILE *f = NULL;
|
| - oflag = find_open_flag(mode, &local_err);
| - if (local_err == NULL) {
| - int fd;
| + oflag = find_open_flag(mode, errp);
| + if (*errp) {
I'd prefer
if (oflag < 0) {
No need for ERRP_GUARD() then, as far as I can tell.
| + goto end;
| + }
|
| /* If the caller wants / allows creation of a new file, we implement it
| * with a two step process: open() + (open() / fchmod()).
| @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
| oflag &= ~(unsigned)O_CREAT;
| fd = open(path, oflag);
| }
| -
| if (fd == -1) {
| - error_setg_errno(&local_err, errno, "failed to open file '%s' "
| - "(mode: '%s')", path, mode);
| - } else {
| + 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(&local_err, errno, "failed to set permission "
| - "0%03o on new file '%s' (mode: '%s')",
| + error_setg_errno(errp, errno,
| + "failed to set permission 0%03o on new file '%s' (mode: '%s')",
| (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
| - } else {
| - FILE *f;
| + goto end;
| + }
|
| 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;
| - }
| + error_setg_errno(errp, errno,
| + "failed to associate stdio stream with file descriptor %d, "
| + "file '%s' (mode: '%s')",
| + fd, path, mode);
| }
|
| +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,
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()
2022-05-05 11:19 ` Markus Armbruster
@ 2022-05-05 11:27 ` Marc-André Lureau
0 siblings, 0 replies; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-05 11:27 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Konstantin Kostiuk, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, open list:Block layer core
[-- Attachment #1: Type: text/plain, Size: 4534 bytes --]
Hi
On Thu, May 5, 2022 at 3:20 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > There is a bit too much branching in the function, this can be
> > simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
>
> Do you mean ERRP_GUARD()?
>
yes
>
> I'm not sure the commit reduces "branching", but it certainly reduces
> nesting, which I find improves readability.
>
ok
>
> > This also helps with the following error handling changes.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qga/commands-posix.c | 124 ++++++++++++++++++++++---------------------
> > 1 file changed, 63 insertions(+), 61 deletions(-)
>
> I think the diff is easier to review with space change ignored:
>
> | diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> | index 78f2f21001..b4b19d3eb4 100644
> | --- a/qga/commands-posix.c
> | +++ b/qga/commands-posix.c
> | @@ -315,12 +315,14 @@ 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;
> | + ERRP_GUARD();
> | + int oflag, fd = -1;
>
> I'd prefer
>
> + ERRP_GUARD();
> int oflag;
> + int fd = -1;
>
>
ok
> because it's slightly less churn, and I dislike variables with and
> without initializer in the same declaration. Matter of taste.
>
> | + FILE *f = NULL;
> |
> | - oflag = find_open_flag(mode, &local_err);
> | - if (local_err == NULL) {
> | - int fd;
> | + oflag = find_open_flag(mode, errp);
> | + if (*errp) {
>
> I'd prefer
>
> if (oflag < 0) {
>
> No need for ERRP_GUARD() then, as far as I can tell.
>
>
"qga: use qemu_open_cloexec() for safe_open_or_create()" expects it to be
non-null though, I can move it there.
> | + goto end;
> | + }
> |
> | /* If the caller wants / allows creation of a new file, we
> implement it
> | * with a two step process: open() + (open() / fchmod()).
> | @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char
> *mode, Error **errp)
> | oflag &= ~(unsigned)O_CREAT;
> | fd = open(path, oflag);
> | }
> | -
> | if (fd == -1) {
> | - error_setg_errno(&local_err, errno, "failed to open file
> '%s' "
> | - "(mode: '%s')", path, mode);
> | - } else {
> | + 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(&local_err, errno, "failed to set
> permission "
> | - "0%03o on new file '%s' (mode: '%s')",
> | + error_setg_errno(errp, errno,
> | + "failed to set permission 0%03o on new file
> '%s' (mode: '%s')",
> | (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> | - } else {
> | - FILE *f;
> | + goto end;
> | + }
> |
> | 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;
> | - }
> | + error_setg_errno(errp, errno,
> | + "failed to associate stdio stream with file
> descriptor %d, "
> | + "file '%s' (mode: '%s')",
> | + fd, path, mode);
> | }
> |
> | +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,
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6725 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()
2022-05-05 8:14 ` [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
@ 2022-05-05 11:32 ` Markus Armbruster
2022-05-13 10:02 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 11:32 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function takes care of setting CLOEXEC, and reporting error.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/commands-posix.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0ef049650e31..8ebc327c5e02 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -370,21 +370,16 @@ 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) {
> + g_clear_pointer(errp, error_free);
Aha, this is where you really need ERRP_GUARD().
Elsewhere, we use
error_free(errp);
errp = NULL;
If one of these two ways is superior, we should use the superior one
everywhere.
If it's a wash, we should stick to the prevalent way.
> 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);
This changes the error message, doesn't it?
Not an objection, just something that might be worth noting in the
commit message.
> 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')",
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()
2022-05-05 8:14 ` [PATCH v2 08/15] qga: throw an Error in ga_channel_open() marcandre.lureau
@ 2022-05-05 11:39 ` Markus Armbruster
2022-05-05 11:49 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 11:39 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Allow for a single point of error reporting, and further refactoring.
This sounds like there is no behavioral change intended. But it looks
like there is change; see below.
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/channel-posix.c | 42 +++++++++++++++++-------------------------
> 1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index a996858e2492..0ce594bc36c2 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 (*errp) {
Recommend
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 (*errp) {
Recommend
if (fd < 0) {
Do you still need ERRP_GUARD() then?
> 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,13 @@ 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)) {
> + error_report_err(err);
> ga_channel_free(c);
> return NULL;
> }
This changes error reporting from g_critical() (which doesn't count as
error reporting in my book) to error_report_err().
How does this affect the program's behavior?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()
2022-05-05 8:14 ` [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
@ 2022-05-05 11:42 ` Markus Armbruster
2022-05-05 11:54 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 11:42 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> 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 0ce594bc36c2..a1262b130145 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 8ebc327c5e02..f82205e25813 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
>
> static void get_nvme_smart(GuestDiskInfo *disk)
> {
> + Error *err = NULL;
> int fd;
> GuestNVMeSmart *smart;
> NvmeSmartLog log = {0};
> @@ -1404,9 +1405,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;
> }
>
> @@ -1737,9 +1739,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;
> }
>
> @@ -1804,7 +1805,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;
> }
> @@ -1864,21 +1865,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) {
Suggest to change build_fs_mount_list() to return bool, in accordance
with the guidance under = Rules = in include/qapi/error.h, then do
if (!build_fs_mount_list(&mounts, errp)) {
No need for ERRP_GUARD() then.
This is not a demand.
> return NULL;
> }
>
> @@ -1890,11 +1890,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;
> }
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible
2022-05-05 8:14 ` [PATCH v2 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
@ 2022-05-05 11:47 ` Markus Armbruster
2022-05-05 11:51 ` Marc-André Lureau
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 11:47 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/unit/test-qga.c | 125 +++++++++++++++---------------------------
> 1 file changed, 45 insertions(+), 80 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index ab0b12a2dd16..53cefc2c2649 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -51,8 +51,11 @@ static void
> fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
> {
> const gchar *extra_arg = data;
> - GError *error = NULL;
> - gchar *cwd, *path, *cmd, **argv = NULL;
> + g_autoptr(GError) error = NULL;
> + g_autofree char *cwd = NULL;
> + g_autofree char *path = NULL;
> + g_autofree char *cmd = NULL;
> + g_auto(GStrv) argv = NULL;
Arranges five variables to be auto-freed, where ...
>
> 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);
... only four were freed before. The extra one is @error. Which can
only be null here, thanks to g_assert_no_error(), can't it?
> }
Didn't look further.
[...]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()
2022-05-05 11:39 ` Markus Armbruster
@ 2022-05-05 11:49 ` Marc-André Lureau
0 siblings, 0 replies; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-05 11:49 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Konstantin Kostiuk, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, open list:Block layer core
[-- Attachment #1: Type: text/plain, Size: 6789 bytes --]
Hi
On Thu, May 5, 2022 at 3:41 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Allow for a single point of error reporting, and further refactoring.
>
> This sounds like there is no behavioral change intended. But it looks
> like there is change; see below.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qga/channel-posix.c | 42 +++++++++++++++++-------------------------
> > 1 file changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index a996858e2492..0ce594bc36c2 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 (*errp) {
>
> Recommend
>
> if (!addr) {
>
>
ok
> > 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 (*errp) {
>
> Recommend
>
> if (fd < 0) {
>
>
ok
> Do you still need ERRP_GUARD() then?
>
It's more future-proof, but could be dropped then.
>
> > 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,13 @@ 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)) {
> > + error_report_err(err);
> > ga_channel_free(c);
> > return NULL;
> > }
>
> This changes error reporting from g_critical() (which doesn't count as
> error reporting in my book) to error_report_err().
>
> How does this affect the program's behavior?
>
Hmm, indeed. I thought because qga uses Error it would have integrated
error_report usage, I am wrong! (so annoying to have both GError and Error)
qga logging is done via glib API, which Error will simply print to stderr.
I will change it to g_critical again then.
(tbh, I prefer v1 with g_auto(Error), but I don't want to debate for such
thing..)
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 9485 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible
2022-05-05 11:47 ` Markus Armbruster
@ 2022-05-05 11:51 ` Marc-André Lureau
2022-05-05 13:39 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-05 11:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
Hi
On Thu, May 5, 2022 at 3:47 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > tests/unit/test-qga.c | 125 +++++++++++++++---------------------------
> > 1 file changed, 45 insertions(+), 80 deletions(-)
> >
> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> > index ab0b12a2dd16..53cefc2c2649 100644
> > --- a/tests/unit/test-qga.c
> > +++ b/tests/unit/test-qga.c
> > @@ -51,8 +51,11 @@ static void
> > fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
> > {
> > const gchar *extra_arg = data;
> > - GError *error = NULL;
> > - gchar *cwd, *path, *cmd, **argv = NULL;
> > + g_autoptr(GError) error = NULL;
> > + g_autofree char *cwd = NULL;
> > + g_autofree char *path = NULL;
> > + g_autofree char *cmd = NULL;
> > + g_auto(GStrv) argv = NULL;
>
> Arranges five variables to be auto-freed, where ...
>
> >
> > 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);
>
> ... only four were freed before. The extra one is @error. Which can
> only be null here, thanks to g_assert_no_error(), can't it?
Right, but for consistency we can have it. I can drop it too.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()
2022-05-05 11:42 ` Markus Armbruster
@ 2022-05-05 11:54 ` Marc-André Lureau
0 siblings, 0 replies; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-05 11:54 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU, Alexander Bulekov, Bandan Das, Thomas Huth, Hanna Reitz,
Konstantin Kostiuk, Stefan Weil, Kevin Wolf, Darren Kenny,
Laurent Vivier, Michael Roth, Paolo Bonzini, Qiuhao Li,
Stefan Hajnoczi, open list:Block layer core
[-- Attachment #1: Type: text/plain, Size: 5860 bytes --]
Hi
On Thu, May 5, 2022 at 3:43 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > 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 0ce594bc36c2..a1262b130145 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 8ebc327c5e02..f82205e25813 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
> >
> > static void get_nvme_smart(GuestDiskInfo *disk)
> > {
> > + Error *err = NULL;
> > int fd;
> > GuestNVMeSmart *smart;
> > NvmeSmartLog log = {0};
> > @@ -1404,9 +1405,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;
> > }
> >
> > @@ -1737,9 +1739,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;
> > }
> >
> > @@ -1804,7 +1805,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;
> > }
> > @@ -1864,21 +1865,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) {
>
> Suggest to change build_fs_mount_list() to return bool, in accordance
> with the guidance under = Rules = in include/qapi/error.h, then do
>
> if (!build_fs_mount_list(&mounts, errp)) {
>
>
ack
> No need for ERRP_GUARD() then.
>
>
This is not a demand.
>
> > return NULL;
> > }
> >
> > @@ -1890,11 +1890,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;
> > }
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 8118 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible
2022-05-05 11:51 ` Marc-André Lureau
@ 2022-05-05 13:39 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2022-05-05 13:39 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Hi
>
> On Thu, May 5, 2022 at 3:47 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> > tests/unit/test-qga.c | 125 +++++++++++++++---------------------------
>> > 1 file changed, 45 insertions(+), 80 deletions(-)
>> >
>> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
>> > index ab0b12a2dd16..53cefc2c2649 100644
>> > --- a/tests/unit/test-qga.c
>> > +++ b/tests/unit/test-qga.c
>> > @@ -51,8 +51,11 @@ static void
>> > fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
>> > {
>> > const gchar *extra_arg = data;
>> > - GError *error = NULL;
>> > - gchar *cwd, *path, *cmd, **argv = NULL;
>> > + g_autoptr(GError) error = NULL;
>> > + g_autofree char *cwd = NULL;
>> > + g_autofree char *path = NULL;
>> > + g_autofree char *cmd = NULL;
>> > + g_auto(GStrv) argv = NULL;
>>
>> Arranges five variables to be auto-freed, where ...
>>
>> >
>> > 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);
>>
>> ... only four were freed before. The extra one is @error. Which can
>> only be null here, thanks to g_assert_no_error(), can't it?
>
> Right, but for consistency we can have it. I can drop it too.
If you want to add no-op frees for consistency, then your commit message
should prepare reviewers for that.
And you should perhaps be prepared for reviewers asking you to split
your patch ;)
Dropping seems the easiest path forward.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()
2022-05-05 11:32 ` Markus Armbruster
@ 2022-05-13 10:02 ` Marc-André Lureau
0 siblings, 0 replies; 35+ messages in thread
From: Marc-André Lureau @ 2022-05-13 10:02 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
Hi
On Thu, May 5, 2022 at 1:33 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > The function takes care of setting CLOEXEC, and reporting error.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qga/commands-posix.c | 11 +++--------
> > 1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0ef049650e31..8ebc327c5e02 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -370,21 +370,16 @@ 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) {
> > + g_clear_pointer(errp, error_free);
>
> Aha, this is where you really need ERRP_GUARD().
>
> Elsewhere, we use
>
> error_free(errp);
> errp = NULL;
>
More like:
error_free(*errp);
*errp = NULL;
compared to:
g_clear_pointer(errp, error_free);
I have a preference ;) but I will switch to the former for now.
> If one of these two ways is superior, we should use the superior one
> everywhere.
>
> If it's a wash, we should stick to the prevalent way.
>
> > 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);
>
> This changes the error message, doesn't it?
>
> Not an objection, just something that might be worth noting in the
> commit message.
>
ok
> > 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')",
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 03/15] tests: make libqmp buildable for win32
2022-05-05 11:08 ` Marc-André Lureau
@ 2022-05-16 5:57 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2022-05-16 5:57 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Alexander Bulekov, Bandan Das, Thomas Huth,
Hanna Reitz, Konstantin Kostiuk, Stefan Weil, Kevin Wolf,
Darren Kenny, Laurent Vivier, Michael Roth, Paolo Bonzini,
Qiuhao Li, Stefan Hajnoczi, qemu-block
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Hi
>
> On Thu, May 5, 2022 at 2:52 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > 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.h | 2 ++
>> > tests/qtest/libqmp.c | 35 +++++++++++++++++++++++++++++------
>> > 2 files changed, 31 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
>> > index 94aa97328a17..772f18b73ba3 100644
>> > --- a/tests/qtest/libqmp.h
>> > +++ b/tests/qtest/libqmp.h
>> > @@ -20,8 +20,10 @@
>> > #include "qapi/qmp/qdict.h"
>> >
>> > QDict *qmp_fd_receive(int fd);
>> > +#ifndef G_OS_WIN32
>>
>> What's the difference between G_OS_WIN32 and _WIN32?
>>
>> We have 10 of the former, but >250 of the latter. If they are
>> effectively the same, we should pick one and stick to it.
>
> There are some subtle differences when compiling for cygwin, in which
> case G_OS_WIN32 is not defined.
>
> I usually pick G_OS_{UNIX,WIN32} defines, mostly for consistency, but
> in many situation _WIN32/WIN32 is fine.
>
> (and we also have CONFIG_WIN32)
I really think we should use one and only one unless there are
differences that *require* more.
Of the three, _WIN32 predominates (numbers appended). Please use _WIN32
unless you can show a need for something else.
$ git-grep -cw G_OS_WIN32
qga/commands.c:5
ui/gtk.c:5
$ git-grep -cw CONFIG_WIN32 master
master:Makefile:1
master:block/meson.build:1
master:chardev/meson.build:1
master:configure:1
master:hw/usb/host-libusb.c:9
master:io/channel-watch.c:5
master:meson.build:1
master:net/meson.build:1
master:qga/meson.build:1
master:scripts/checkpatch.pl:1
master:target/i386/hax/hax-i386.h:2
master:target/i386/hax/meson.build:1
master:ui/gtk.c:1
master:ui/meson.build:2
master:ui/sdl2.c:1
master:util/meson.build:5
master:util/sys_membarrier.c:1
$ git-grep -cw CONFIG_WIN32 master
master:Makefile:1
master:block/meson.build:1
master:chardev/meson.build:1
master:configure:1
master:hw/usb/host-libusb.c:9
master:io/channel-watch.c:5
master:meson.build:1
master:net/meson.build:1
master:qga/meson.build:1
master:scripts/checkpatch.pl:1
master:target/i386/hax/hax-i386.h:2
master:target/i386/hax/meson.build:1
master:ui/gtk.c:1
master:ui/meson.build:2
master:ui/sdl2.c:1
master:util/meson.build:5
master:util/sys_membarrier.c:1
$ git-grep -cw _WIN32 master
master:accel/tcg/cpu-exec.c:1
master:accel/tcg/tcg-accel-ops-mttcg.c:1
master:accel/tcg/tcg-accel-ops-rr.c:1
master:audio/sdlaudio.c:3
master:block.c:6
master:block/nfs.c:5
master:block/vvfat.c:1
master:chardev/char-file.c:3
master:chardev/char-pipe.c:4
master:chardev/char-serial.c:4
master:chardev/char-socket.c:1
master:chardev/char-stdio.c:5
master:configure:1
master:contrib/elf2dmp/kdbg.h:1
master:contrib/elf2dmp/pdb.h:1
master:contrib/elf2dmp/pe.h:1
master:crypto/pbkdf.c:2
master:crypto/random-platform.c:3
master:gdbstub.c:2
master:include/block/block_int-common.h:1
master:include/block/raw-aio.h:1
master:include/exec/ram_addr.h:2
master:include/hw/core/cpu.h:1
master:include/io/channel.h:1
master:include/qapi/error.h:1
master:include/qemu/compiler.h:2
master:include/qemu/event_notifier.h:2
master:include/qemu/main-loop.h:1
master:include/qemu/osdep.h:6
master:include/qemu/qemu-plugin.h:1
master:include/qemu/sockets.h:2
master:include/qemu/thread.h:1
master:include/qemu/timer.h:1
master:io/channel.c:1
master:monitor/misc.c:1
master:nbd/nbd-internal.h:1
master:net/net.c:1
master:qemu-io.c:2
master:qemu-options.hx:6
master:qga/guest-agent-core.h:1
master:qga/main.c:21
master:scripts/cocci-macro-file.h:1
master:scripts/codeconverter/codeconverter/test_regexps.py:1
master:softmmu/cpus.c:2
master:softmmu/physmem.c:4
master:softmmu/vl.c:3
master:stubs/is-daemonized.c:1
master:subprojects/libvhost-user/libvhost-user.h:1
master:target/i386/hax/hax-accel-ops.c:1
master:target/i386/whpx/whpx-accel-ops.c:1
master:target/m68k/m68k-semi.c:1
master:target/mips/tcg/sysemu/mips-semi.c:4
master:target/nios2/nios2-semi.c:1
master:tcg/region.c:1
master:tests/qtest/virtio-net-test.c:2
master:tests/unit/test-char.c:6
master:tests/unit/test-crypto-block.c:2
master:tests/unit/test-crypto-pbkdf.c:2
master:tests/unit/test-io-channel-file.c:4
master:tests/unit/test-io-channel-socket.c:4
master:tests/unit/test-iov.c:1
master:tests/unit/test-replication.c:3
master:tests/unit/test-util-sockets.c:2
master:trace/simple.c:3
master:ui/curses.c:3
master:util/cacheinfo.c:1
master:util/error.c:1
master:util/main-loop.c:4
master:util/osdep.c:8
master:util/qemu-sockets.c:2
master:util/qemu-timer-common.c:1
master:util/systemd.c:2
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2022-05-16 6:01 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-05 8:14 [PATCH v2 00/15] Misc cleanups marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir() marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 03/15] tests: make libqmp buildable for win32 marcandre.lureau
2022-05-05 10:52 ` Markus Armbruster
2022-05-05 11:08 ` Marc-André Lureau
2022-05-16 5:57 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 04/15] include: adjust header guards after renaming marcandre.lureau
2022-05-05 10:58 ` Markus Armbruster
2022-05-05 8:14 ` [PATCH v2 05/15] qga: flatten safe_open_or_create() marcandre.lureau
2022-05-05 11:19 ` Markus Armbruster
2022-05-05 11:27 ` Marc-André Lureau
2022-05-05 8:14 ` [PATCH v2 06/15] osdep: export qemu_open_cloexec() marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create() marcandre.lureau
2022-05-05 11:32 ` Markus Armbruster
2022-05-13 10:02 ` Marc-André Lureau
2022-05-05 8:14 ` [PATCH v2 08/15] qga: throw an Error in ga_channel_open() marcandre.lureau
2022-05-05 11:39 ` Markus Armbruster
2022-05-05 11:49 ` Marc-André Lureau
2022-05-05 8:14 ` [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec() marcandre.lureau
2022-05-05 11:42 ` Markus Armbruster
2022-05-05 11:54 ` Marc-André Lureau
2022-05-05 8:14 ` [PATCH v2 10/15] test/qga: use G_TEST_DIR to locate os-release test file marcandre.lureau
2022-05-05 8:14 ` [PATCH v2 11/15] qga/wixl: prefer variables over environment marcandre.lureau
2022-05-05 8:25 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 12/15] qga/wixl: require Mingw_bin marcandre.lureau
2022-05-05 8:26 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 13/15] qga/wixl: simplify some pre-processing marcandre.lureau
2022-05-05 8:26 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir marcandre.lureau
2022-05-05 8:28 ` Konstantin Kostiuk
2022-05-05 8:14 ` [PATCH v2 15/15] test/qga: use g_auto wherever sensible marcandre.lureau
2022-05-05 11:47 ` Markus Armbruster
2022-05-05 11:51 ` Marc-André Lureau
2022-05-05 13:39 ` Markus Armbruster
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.