* [PATCH v2 01/15] error-report: misc comment fix
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 02/15] error-report: introduce "detailed" variable marcandre.lureau
` (13 subsequent siblings)
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Missed in commit beeb175c0d "util/qemu-error: Rename error_print_loc()
to be more generic".
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
util/error-report.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/error-report.c b/util/error-report.c
index 5edb2e604061..98f242b75bbf 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -390,7 +390,7 @@ void error_init(const char *argv0)
{
const char *p = strrchr(argv0, '/');
- /* Set the program name for error_print_loc(). */
+ /* Set the program name for print_loc(). */
g_set_prgname(p ? p + 1 : argv0);
/*
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 02/15] error-report: introduce "detailed" variable
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 01/15] error-report: misc comment fix marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 03/15] error-report: simplify print_loc() marcandre.lureau
` (12 subsequent siblings)
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Let's use a more explicit variable "detailed" instead of calling
monitor_cur() multiple times.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
util/error-report.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/util/error-report.c b/util/error-report.c
index 98f242b75bbf..893da10f19bc 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -195,16 +195,17 @@ real_time_iso8601(void)
*/
static void vreport(report_type type, const char *fmt, va_list ap)
{
+ bool detailed = !monitor_cur();
gchar *timestr;
- if (message_with_timestamp && !monitor_cur()) {
+ if (message_with_timestamp && detailed) {
timestr = real_time_iso8601();
error_printf("%s ", timestr);
g_free(timestr);
}
/* Only prepend guest name if -msg guest-name and -name guest=... are set */
- if (error_with_guestname && error_guest_name && !monitor_cur()) {
+ if (error_with_guestname && error_guest_name && detailed) {
error_printf("%s ", error_guest_name);
}
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 03/15] error-report: simplify print_loc()
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 01/15] error-report: misc comment fix marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 02/15] error-report: introduce "detailed" variable marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 04/15] error-report: introduce overridable error_is_detailed() marcandre.lureau
` (11 subsequent siblings)
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Pass the program name as "prefix" argument to print_loc() if printing
with "details". This allows to get rid of monitor_cur() call in
print_loc().
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
util/error-report.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/error-report.c b/util/error-report.c
index 893da10f19bc..c43227a975e2 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -138,14 +138,14 @@ void loc_set_file(const char *fname, int lno)
/*
* Print current location to current monitor if we have one, else to stderr.
*/
-static void print_loc(void)
+static void print_loc(const char *prefix)
{
const char *sep = "";
int i;
const char *const *argp;
- if (!monitor_cur() && g_get_prgname()) {
- error_printf("%s:", g_get_prgname());
+ if (prefix) {
+ error_printf("%s:", prefix);
sep = " ";
}
switch (cur_loc->kind) {
@@ -209,7 +209,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
error_printf("%s ", error_guest_name);
}
- print_loc();
+ print_loc(detailed ? g_get_prgname() : NULL);
switch (type) {
case REPORT_TYPE_ERROR:
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 04/15] error-report: introduce overridable error_is_detailed()
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (2 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 03/15] error-report: simplify print_loc() marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 15:02 ` Warner Losh
2022-07-19 7:24 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp() marcandre.lureau
` (10 subsequent siblings)
14 siblings, 2 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Remove the direct dependency from error-report to monitor code.
This will allow to move error-report to a subproject.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qemu/error-report.h | 2 ++
softmmu/vl.c | 5 +++++
stubs/error-is-detailed.c | 7 +++++++
util/error-report.c | 3 +--
stubs/meson.build | 1 +
5 files changed, 16 insertions(+), 2 deletions(-)
create mode 100644 stubs/error-is-detailed.c
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3ae2357fda54..6ab25d458350 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -30,6 +30,8 @@ void loc_set_none(void);
void loc_set_cmdline(char **argv, int idx, int cnt);
void loc_set_file(const char *fname, int lno);
+bool error_is_detailed(void);
+
int error_vprintf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
int error_printf(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3f264d4b0930..f94efc56e9d6 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2589,6 +2589,11 @@ void qmp_x_exit_preconfig(Error **errp)
}
}
+bool error_is_detailed(void)
+{
+ return !monitor_cur();
+}
+
void qemu_init(int argc, char **argv, char **envp)
{
QemuOpts *opts;
diff --git a/stubs/error-is-detailed.c b/stubs/error-is-detailed.c
new file mode 100644
index 000000000000..c47cd236932f
--- /dev/null
+++ b/stubs/error-is-detailed.c
@@ -0,0 +1,7 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+
+bool error_is_detailed(void)
+{
+ return TRUE;
+}
diff --git a/util/error-report.c b/util/error-report.c
index c43227a975e2..4d1d66fc0650 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -11,7 +11,6 @@
*/
#include "qemu/osdep.h"
-#include "monitor/monitor.h"
#include "qemu/error-report.h"
/*
@@ -195,7 +194,7 @@ real_time_iso8601(void)
*/
static void vreport(report_type type, const char *fmt, va_list ap)
{
- bool detailed = !monitor_cur();
+ bool detailed = error_is_detailed();
gchar *timestr;
if (message_with_timestamp && detailed) {
diff --git a/stubs/meson.build b/stubs/meson.build
index d8f3fd5c44f2..0f3a782824f9 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -9,6 +9,7 @@ stub_ss.add(files('cpus-get-virtual-clock.c'))
stub_ss.add(files('qemu-timer-notify-cb.c'))
stub_ss.add(files('icount.c'))
stub_ss.add(files('dump.c'))
+stub_ss.add(files('error-is-detailed.c'))
stub_ss.add(files('error-printf.c'))
stub_ss.add(files('fdset.c'))
stub_ss.add(files('gdbstub.c'))
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 04/15] error-report: introduce overridable error_is_detailed()
2022-07-12 9:35 ` [PATCH v2 04/15] error-report: introduce overridable error_is_detailed() marcandre.lureau
@ 2022-07-12 15:02 ` Warner Losh
2022-07-19 7:24 ` Markus Armbruster
1 sibling, 0 replies; 44+ messages in thread
From: Warner Losh @ 2022-07-12 15:02 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU Developers, Eric Blake, Cleber Rosa,
open list:Block layer core, Paolo Bonzini, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]
On Tue, Jul 12, 2022 at 3:36 AM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove the direct dependency from error-report to monitor code.
> This will allow to move error-report to a subproject.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qemu/error-report.h | 2 ++
> softmmu/vl.c | 5 +++++
> stubs/error-is-detailed.c | 7 +++++++
> util/error-report.c | 3 +--
> stubs/meson.build | 1 +
> 5 files changed, 16 insertions(+), 2 deletions(-)
> create mode 100644 stubs/error-is-detailed.c
>
Reviewed-by: Warner Losh <imp@bsdimp.com>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..6ab25d458350 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -30,6 +30,8 @@ void loc_set_none(void);
> void loc_set_cmdline(char **argv, int idx, int cnt);
> void loc_set_file(const char *fname, int lno);
>
> +bool error_is_detailed(void);
> +
> int error_vprintf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
> int error_printf(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 3f264d4b0930..f94efc56e9d6 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2589,6 +2589,11 @@ void qmp_x_exit_preconfig(Error **errp)
> }
> }
>
> +bool error_is_detailed(void)
> +{
> + return !monitor_cur();
> +}
> +
> void qemu_init(int argc, char **argv, char **envp)
> {
> QemuOpts *opts;
> diff --git a/stubs/error-is-detailed.c b/stubs/error-is-detailed.c
> new file mode 100644
> index 000000000000..c47cd236932f
> --- /dev/null
> +++ b/stubs/error-is-detailed.c
> @@ -0,0 +1,7 @@
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +
> +bool error_is_detailed(void)
> +{
> + return TRUE;
> +}
> diff --git a/util/error-report.c b/util/error-report.c
> index c43227a975e2..4d1d66fc0650 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -11,7 +11,6 @@
> */
>
> #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
> #include "qemu/error-report.h"
>
> /*
> @@ -195,7 +194,7 @@ real_time_iso8601(void)
> */
> static void vreport(report_type type, const char *fmt, va_list ap)
> {
> - bool detailed = !monitor_cur();
> + bool detailed = error_is_detailed();
> gchar *timestr;
>
> if (message_with_timestamp && detailed) {
> diff --git a/stubs/meson.build b/stubs/meson.build
> index d8f3fd5c44f2..0f3a782824f9 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -9,6 +9,7 @@ stub_ss.add(files('cpus-get-virtual-clock.c'))
> stub_ss.add(files('qemu-timer-notify-cb.c'))
> stub_ss.add(files('icount.c'))
> stub_ss.add(files('dump.c'))
> +stub_ss.add(files('error-is-detailed.c'))
> stub_ss.add(files('error-printf.c'))
> stub_ss.add(files('fdset.c'))
> stub_ss.add(files('gdbstub.c'))
> --
> 2.37.0.rc0
>
>
[-- Attachment #2: Type: text/html, Size: 4082 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 04/15] error-report: introduce overridable error_is_detailed()
2022-07-12 9:35 ` [PATCH v2 04/15] error-report: introduce overridable error_is_detailed() marcandre.lureau
2022-07-12 15:02 ` Warner Losh
@ 2022-07-19 7:24 ` Markus Armbruster
1 sibling, 0 replies; 44+ messages in thread
From: Markus Armbruster @ 2022-07-19 7:24 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove the direct dependency from error-report to monitor code.
> This will allow to move error-report to a subproject.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp()
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (3 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 04/15] error-report: introduce overridable error_is_detailed() marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-19 7:24 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor marcandre.lureau
` (9 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
stubs/error-printf.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index 0e326d801059..1afa0f62ca26 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
}
return vfprintf(stderr, fmt, ap);
}
-
-int error_vprintf_unless_qmp(const char *fmt, va_list ap)
-{
- return error_vprintf(fmt, ap);
-}
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp()
2022-07-12 9:35 ` [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp() marcandre.lureau
@ 2022-07-19 7:24 ` Markus Armbruster
0 siblings, 0 replies; 44+ messages in thread
From: Markus Armbruster @ 2022-07-19 7:24 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> stubs/error-printf.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> index 0e326d801059..1afa0f62ca26 100644
> --- a/stubs/error-printf.c
> +++ b/stubs/error-printf.c
> @@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
> }
> return vfprintf(stderr, fmt, ap);
> }
> -
> -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> -{
> - return error_vprintf(fmt, ap);
> -}
Easy enough to add back should we ever need it.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (4 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 05/15] stubs: remove needless error_vprintf_unless_qmp() marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-08-02 10:58 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 07/15] scripts/qapi-gen: add -i option marcandre.lureau
` (8 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Make QMP-dispatch code free from QEMU-specific OOB dispatch/async
coroutine handling. This will allow to move the base code to
qemu-common, and clear other users from potential mis-ususe (QGA doesn't
have OOB or coroutine).
To do that, introduce an optional callback QmpDispatchRun called when a
QMP command should be run, to allow QEMU to override the default
behaviour.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qapi/qmp/dispatch.h | 7 ++--
monitor/qmp.c | 68 ++++++++++++++++++++++++++++++++++++-
qapi/qmp-dispatch.c | 64 +++-------------------------------
qga/main.c | 2 +-
tests/unit/test-qmp-cmds.c | 6 ++--
5 files changed, 81 insertions(+), 66 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1e4240fd0dbc..b659da613f2e 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -14,7 +14,6 @@
#ifndef QAPI_QMP_DISPATCH_H
#define QAPI_QMP_DISPATCH_H
-#include "monitor/monitor.h"
#include "qemu/queue.h"
typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -41,6 +40,10 @@ typedef struct QmpCommand
typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
+typedef void (QmpDispatchRun)(bool oob, const QmpCommand *cmd,
+ QDict *args, QObject **ret, Error **errp,
+ void *run_data);
+
void qmp_register_command(QmpCommandList *cmds, const char *name,
QmpCommandFunc *fn, QmpCommandOptions options,
unsigned special_features);
@@ -56,7 +59,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
bool qmp_has_success_response(const QmpCommand *cmd);
QDict *qmp_error_response(Error *err);
QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
- bool allow_oob, Monitor *cur_mon);
+ bool allow_oob, QmpDispatchRun run_cb, void *run_data);
bool qmp_is_oob(const QDict *dict);
typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 092c527b6fc9..f8dec97c96bb 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -132,6 +132,72 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
}
}
+typedef struct QmpDispatchBH {
+ const QmpCommand *cmd;
+ Monitor *cur_mon;
+ QDict *args;
+ QObject **ret;
+ Error **errp;
+ Coroutine *co;
+} QmpDispatchBH;
+
+static void do_qmp_dispatch_bh(void *opaque)
+{
+ QmpDispatchBH *data = opaque;
+
+ assert(monitor_cur() == NULL);
+ monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
+ data->cmd->fn(data->args, data->ret, data->errp);
+ monitor_set_cur(qemu_coroutine_self(), NULL);
+ aio_co_wake(data->co);
+}
+
+/*
+ * Runs outside of coroutine context for OOB commands, but in coroutine
+ * context for everything else.
+ */
+static void qmp_dispatch_run(bool oob, const QmpCommand *cmd,
+ QDict *args, QObject **ret, Error **errp,
+ void *run_data)
+{
+ Monitor *cur_mon = run_data;
+
+ assert(!(oob && qemu_in_coroutine()));
+ assert(monitor_cur() == NULL);
+
+ if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
+ monitor_set_cur(qemu_coroutine_self(), cur_mon);
+ cmd->fn(args, ret, errp);
+ monitor_set_cur(qemu_coroutine_self(), NULL);
+ } else {
+ /*
+ * Actual context doesn't match the one the command needs.
+ *
+ * Case 1: we are in coroutine context, but command does not
+ * have QCO_COROUTINE. We need to drop out of coroutine
+ * context for executing it.
+ *
+ * Case 2: we are outside coroutine context, but command has
+ * QCO_COROUTINE. Can't actually happen, because we get here
+ * outside coroutine context only when executing a command
+ * out of band, and OOB commands never have QCO_COROUTINE.
+ */
+ assert(!oob && qemu_in_coroutine() && !(cmd->options & QCO_COROUTINE));
+
+ QmpDispatchBH data = {
+ .cur_mon = cur_mon,
+ .cmd = cmd,
+ .args = args,
+ .ret = ret,
+ .errp = errp,
+ .co = qemu_coroutine_self(),
+ };
+ aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
+ &data);
+ qemu_coroutine_yield();
+ }
+}
+
/*
* Runs outside of coroutine context for OOB commands, but in
* coroutine context for everything else.
@@ -142,7 +208,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
QDict *error;
rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
- &mon->common);
+ qmp_dispatch_run, &mon->common);
if (mon->commands == &qmp_cap_negotiation_commands) {
error = qdict_get_qdict(rsp, "error");
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 0990873ec8ec..342b13d7ebbd 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -13,7 +13,6 @@
#include "qemu/osdep.h"
-#include "block/aio.h"
#include "qapi/compat-policy.h"
#include "qapi/error.h"
#include "qapi/qmp/dispatch.h"
@@ -22,8 +21,6 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qmp/qbool.h"
-#include "qemu/coroutine.h"
-#include "qemu/main-loop.h"
Visitor *qobject_input_visitor_new_qmp(QObject *obj)
{
@@ -110,32 +107,8 @@ bool qmp_is_oob(const QDict *dict)
&& !qdict_haskey(dict, "execute");
}
-typedef struct QmpDispatchBH {
- const QmpCommand *cmd;
- Monitor *cur_mon;
- QDict *args;
- QObject **ret;
- Error **errp;
- Coroutine *co;
-} QmpDispatchBH;
-
-static void do_qmp_dispatch_bh(void *opaque)
-{
- QmpDispatchBH *data = opaque;
-
- assert(monitor_cur() == NULL);
- monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
- data->cmd->fn(data->args, data->ret, data->errp);
- monitor_set_cur(qemu_coroutine_self(), NULL);
- aio_co_wake(data->co);
-}
-
-/*
- * Runs outside of coroutine context for OOB commands, but in coroutine
- * context for everything else.
- */
QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
- bool allow_oob, Monitor *cur_mon)
+ bool allow_oob, QmpDispatchRun run_cb, void *run_data)
{
Error *err = NULL;
bool oob;
@@ -203,39 +176,12 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
qobject_ref(args);
}
- assert(!(oob && qemu_in_coroutine()));
- assert(monitor_cur() == NULL);
- if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
- monitor_set_cur(qemu_coroutine_self(), cur_mon);
- cmd->fn(args, &ret, &err);
- monitor_set_cur(qemu_coroutine_self(), NULL);
+ if (run_cb) {
+ run_cb(oob, cmd, args, &ret, &err, run_data);
} else {
- /*
- * Actual context doesn't match the one the command needs.
- *
- * Case 1: we are in coroutine context, but command does not
- * have QCO_COROUTINE. We need to drop out of coroutine
- * context for executing it.
- *
- * Case 2: we are outside coroutine context, but command has
- * QCO_COROUTINE. Can't actually happen, because we get here
- * outside coroutine context only when executing a command
- * out of band, and OOB commands never have QCO_COROUTINE.
- */
- assert(!oob && qemu_in_coroutine() && !(cmd->options & QCO_COROUTINE));
-
- QmpDispatchBH data = {
- .cur_mon = cur_mon,
- .cmd = cmd,
- .args = args,
- .ret = &ret,
- .errp = &err,
- .co = qemu_coroutine_self(),
- };
- aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
- &data);
- qemu_coroutine_yield();
+ cmd->fn(args, &ret, &err);
}
+
qobject_unref(args);
if (err) {
/* or assert(!ret) after reviewing all handlers: */
diff --git a/qga/main.c b/qga/main.c
index c373fec3ee69..fb7d673bea9f 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -569,7 +569,7 @@ static void process_event(void *opaque, QObject *obj, Error *err)
}
g_debug("processing command");
- rsp = qmp_dispatch(&ga_commands, obj, false, NULL);
+ rsp = qmp_dispatch(&ga_commands, obj, false, NULL, NULL);
end:
ret = send_response(s, rsp);
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 6085c099950b..abe67a9bd880 100644
--- a/tests/unit/test-qmp-cmds.c
+++ b/tests/unit/test-qmp-cmds.c
@@ -150,7 +150,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
req = qdict_from_vjsonf_nofail(template, ap);
va_end(ap);
- resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL, NULL);
g_assert(resp);
ret = qdict_get(resp, "return");
g_assert(ret);
@@ -173,7 +173,7 @@ static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
req = qdict_from_vjsonf_nofail(template, ap);
va_end(ap);
- resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL, NULL);
g_assert(resp);
error = qdict_get_qdict(resp, "error");
g_assert(error);
@@ -229,7 +229,7 @@ static void test_dispatch_cmd_success_response(void)
QDict *resp;
qdict_put_str(req, "execute", "cmd-success-response");
- resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL);
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL, NULL);
g_assert_null(resp);
qobject_unref(req);
}
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor
2022-07-12 9:35 ` [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor marcandre.lureau
@ 2022-08-02 10:58 ` Markus Armbruster
2022-08-02 11:19 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-08-02 10:58 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Make QMP-dispatch code free from QEMU-specific OOB dispatch/async
> coroutine handling. This will allow to move the base code to
> qemu-common, and clear other users from potential mis-ususe (QGA doesn't
> have OOB or coroutine).
I trust the utilty of such a move will become clear later in this
series.
>
> To do that, introduce an optional callback QmpDispatchRun called when a
> QMP command should be run, to allow QEMU to override the default
> behaviour.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 7 ++--
> monitor/qmp.c | 68 ++++++++++++++++++++++++++++++++++++-
> qapi/qmp-dispatch.c | 64 +++-------------------------------
> qga/main.c | 2 +-
> tests/unit/test-qmp-cmds.c | 6 ++--
> 5 files changed, 81 insertions(+), 66 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1e4240fd0dbc..b659da613f2e 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -14,7 +14,6 @@
> #ifndef QAPI_QMP_DISPATCH_H
> #define QAPI_QMP_DISPATCH_H
>
> -#include "monitor/monitor.h"
> #include "qemu/queue.h"
>
> typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> @@ -41,6 +40,10 @@ typedef struct QmpCommand
>
> typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
>
> +typedef void (QmpDispatchRun)(bool oob, const QmpCommand *cmd,
> + QDict *args, QObject **ret, Error **errp,
> + void *run_data);
> +
> void qmp_register_command(QmpCommandList *cmds, const char *name,
> QmpCommandFunc *fn, QmpCommandOptions options,
> unsigned special_features);
> @@ -56,7 +59,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
> bool qmp_has_success_response(const QmpCommand *cmd);
> QDict *qmp_error_response(Error *err);
> QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> - bool allow_oob, Monitor *cur_mon);
> + bool allow_oob, QmpDispatchRun run_cb, void *run_data);
> bool qmp_is_oob(const QDict *dict);
>
> typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 092c527b6fc9..f8dec97c96bb 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -132,6 +132,72 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
> }
> }
>
> +typedef struct QmpDispatchBH {
> + const QmpCommand *cmd;
> + Monitor *cur_mon;
> + QDict *args;
> + QObject **ret;
> + Error **errp;
> + Coroutine *co;
> +} QmpDispatchBH;
> +
> +static void do_qmp_dispatch_bh(void *opaque)
> +{
> + QmpDispatchBH *data = opaque;
> +
> + assert(monitor_cur() == NULL);
> + monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> + data->cmd->fn(data->args, data->ret, data->errp);
> + monitor_set_cur(qemu_coroutine_self(), NULL);
> + aio_co_wake(data->co);
> +}
> +
> +/*
> + * Runs outside of coroutine context for OOB commands, but in coroutine
> + * context for everything else.
> + */
> +static void qmp_dispatch_run(bool oob, const QmpCommand *cmd,
> + QDict *args, QObject **ret, Error **errp,
> + void *run_data)
> +{
> + Monitor *cur_mon = run_data;
> +
> + assert(!(oob && qemu_in_coroutine()));
> + assert(monitor_cur() == NULL);
> +
> + if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
> + monitor_set_cur(qemu_coroutine_self(), cur_mon);
> + cmd->fn(args, ret, errp);
> + monitor_set_cur(qemu_coroutine_self(), NULL);
> + } else {
> + /*
> + * Actual context doesn't match the one the command needs.
> + *
> + * Case 1: we are in coroutine context, but command does not
> + * have QCO_COROUTINE. We need to drop out of coroutine
> + * context for executing it.
> + *
> + * Case 2: we are outside coroutine context, but command has
> + * QCO_COROUTINE. Can't actually happen, because we get here
> + * outside coroutine context only when executing a command
> + * out of band, and OOB commands never have QCO_COROUTINE.
> + */
> + assert(!oob && qemu_in_coroutine() && !(cmd->options & QCO_COROUTINE));
> +
> + QmpDispatchBH data = {
> + .cur_mon = cur_mon,
> + .cmd = cmd,
> + .args = args,
> + .ret = ret,
> + .errp = errp,
> + .co = qemu_coroutine_self(),
> + };
> + aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
> + &data);
> + qemu_coroutine_yield();
> + }
> +}
> +
> /*
> * Runs outside of coroutine context for OOB commands, but in
> * coroutine context for everything else.
> @@ -142,7 +208,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
> QDict *error;
>
> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
> - &mon->common);
> + qmp_dispatch_run, &mon->common);
>
> if (mon->commands == &qmp_cap_negotiation_commands) {
> error = qdict_get_qdict(rsp, "error");
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 0990873ec8ec..342b13d7ebbd 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -13,7 +13,6 @@
>
> #include "qemu/osdep.h"
>
> -#include "block/aio.h"
> #include "qapi/compat-policy.h"
> #include "qapi/error.h"
> #include "qapi/qmp/dispatch.h"
> @@ -22,8 +21,6 @@
> #include "qapi/qobject-input-visitor.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qapi/qmp/qbool.h"
> -#include "qemu/coroutine.h"
> -#include "qemu/main-loop.h"
>
> Visitor *qobject_input_visitor_new_qmp(QObject *obj)
> {
> @@ -110,32 +107,8 @@ bool qmp_is_oob(const QDict *dict)
> && !qdict_haskey(dict, "execute");
> }
>
> -typedef struct QmpDispatchBH {
> - const QmpCommand *cmd;
> - Monitor *cur_mon;
> - QDict *args;
> - QObject **ret;
> - Error **errp;
> - Coroutine *co;
> -} QmpDispatchBH;
> -
> -static void do_qmp_dispatch_bh(void *opaque)
> -{
> - QmpDispatchBH *data = opaque;
> -
> - assert(monitor_cur() == NULL);
> - monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> - data->cmd->fn(data->args, data->ret, data->errp);
> - monitor_set_cur(qemu_coroutine_self(), NULL);
> - aio_co_wake(data->co);
> -}
> -
> -/*
> - * Runs outside of coroutine context for OOB commands, but in coroutine
> - * context for everything else.
> - */
> QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> - bool allow_oob, Monitor *cur_mon)
> + bool allow_oob, QmpDispatchRun run_cb, void *run_data)
> {
> Error *err = NULL;
> bool oob;
> @@ -203,39 +176,12 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> qobject_ref(args);
> }
>
> - assert(!(oob && qemu_in_coroutine()));
> - assert(monitor_cur() == NULL);
> - if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
> - monitor_set_cur(qemu_coroutine_self(), cur_mon);
> - cmd->fn(args, &ret, &err);
> - monitor_set_cur(qemu_coroutine_self(), NULL);
> + if (run_cb) {
> + run_cb(oob, cmd, args, &ret, &err, run_data);
> } else {
> - /*
> - * Actual context doesn't match the one the command needs.
> - *
> - * Case 1: we are in coroutine context, but command does not
> - * have QCO_COROUTINE. We need to drop out of coroutine
> - * context for executing it.
> - *
> - * Case 2: we are outside coroutine context, but command has
> - * QCO_COROUTINE. Can't actually happen, because we get here
> - * outside coroutine context only when executing a command
> - * out of band, and OOB commands never have QCO_COROUTINE.
> - */
> - assert(!oob && qemu_in_coroutine() && !(cmd->options & QCO_COROUTINE));
> -
> - QmpDispatchBH data = {
> - .cur_mon = cur_mon,
> - .cmd = cmd,
> - .args = args,
> - .ret = &ret,
> - .errp = &err,
> - .co = qemu_coroutine_self(),
> - };
> - aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
> - &data);
> - qemu_coroutine_yield();
> + cmd->fn(args, &ret, &err);
> }
> +
> qobject_unref(args);
> if (err) {
> /* or assert(!ret) after reviewing all handlers: */
A callback works, but note that each program's function is fixed (the
simple and common function is inlined, but that's just for convenience).
We could use the linker instead. We already do for
qmp_command_available(), and the patch doesn't change that.
Perhaps a layering argument could be made for callbacks. Before the
series, monitor/qmp.c's monitor_qmp_dispatch() calls
qapi/qmp-dispatch.c's qmp_dispatch(), which calls a few functions from
monitor/. However, consistency seems desirable.
What do you think?
[...]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor
2022-08-02 10:58 ` Markus Armbruster
@ 2022-08-02 11:19 ` Marc-André Lureau
2022-08-02 12:21 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-02 11:19 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 10956 bytes --]
Hi
On Tue, Aug 2, 2022 at 3:04 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Make QMP-dispatch code free from QEMU-specific OOB dispatch/async
> > coroutine handling. This will allow to move the base code to
> > qemu-common, and clear other users from potential mis-ususe (QGA doesn't
>
misuse :)
> have OOB or coroutine).
>
> I trust the utilty of such a move will become clear later in this
> series.
>
> >
> > To do that, introduce an optional callback QmpDispatchRun called when a
> > QMP command should be run, to allow QEMU to override the default
> > behaviour.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > include/qapi/qmp/dispatch.h | 7 ++--
> > monitor/qmp.c | 68 ++++++++++++++++++++++++++++++++++++-
> > qapi/qmp-dispatch.c | 64 +++-------------------------------
> > qga/main.c | 2 +-
> > tests/unit/test-qmp-cmds.c | 6 ++--
> > 5 files changed, 81 insertions(+), 66 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 1e4240fd0dbc..b659da613f2e 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -14,7 +14,6 @@
> > #ifndef QAPI_QMP_DISPATCH_H
> > #define QAPI_QMP_DISPATCH_H
> >
> > -#include "monitor/monitor.h"
> > #include "qemu/queue.h"
> >
> > typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> > @@ -41,6 +40,10 @@ typedef struct QmpCommand
> >
> > typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
> >
> > +typedef void (QmpDispatchRun)(bool oob, const QmpCommand *cmd,
> > + QDict *args, QObject **ret, Error **errp,
> > + void *run_data);
> > +
> > void qmp_register_command(QmpCommandList *cmds, const char *name,
> > QmpCommandFunc *fn, QmpCommandOptions options,
> > unsigned special_features);
> > @@ -56,7 +59,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
> > bool qmp_has_success_response(const QmpCommand *cmd);
> > QDict *qmp_error_response(Error *err);
> > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> > - bool allow_oob, Monitor *cur_mon);
> > + bool allow_oob, QmpDispatchRun run_cb, void
> *run_data);
> > bool qmp_is_oob(const QDict *dict);
> >
> > typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void
> *opaque);
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 092c527b6fc9..f8dec97c96bb 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -132,6 +132,72 @@ static void monitor_qmp_respond(MonitorQMP *mon,
> QDict *rsp)
> > }
> > }
> >
> > +typedef struct QmpDispatchBH {
> > + const QmpCommand *cmd;
> > + Monitor *cur_mon;
> > + QDict *args;
> > + QObject **ret;
> > + Error **errp;
> > + Coroutine *co;
> > +} QmpDispatchBH;
> > +
> > +static void do_qmp_dispatch_bh(void *opaque)
> > +{
> > + QmpDispatchBH *data = opaque;
> > +
> > + assert(monitor_cur() == NULL);
> > + monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> > + data->cmd->fn(data->args, data->ret, data->errp);
> > + monitor_set_cur(qemu_coroutine_self(), NULL);
> > + aio_co_wake(data->co);
> > +}
> > +
> > +/*
> > + * Runs outside of coroutine context for OOB commands, but in coroutine
> > + * context for everything else.
> > + */
> > +static void qmp_dispatch_run(bool oob, const QmpCommand *cmd,
> > + QDict *args, QObject **ret, Error **errp,
> > + void *run_data)
> > +{
> > + Monitor *cur_mon = run_data;
> > +
> > + assert(!(oob && qemu_in_coroutine()));
> > + assert(monitor_cur() == NULL);
> > +
> > + if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
> > + monitor_set_cur(qemu_coroutine_self(), cur_mon);
> > + cmd->fn(args, ret, errp);
> > + monitor_set_cur(qemu_coroutine_self(), NULL);
> > + } else {
> > + /*
> > + * Actual context doesn't match the one the command needs.
> > + *
> > + * Case 1: we are in coroutine context, but command does not
> > + * have QCO_COROUTINE. We need to drop out of coroutine
> > + * context for executing it.
> > + *
> > + * Case 2: we are outside coroutine context, but command has
> > + * QCO_COROUTINE. Can't actually happen, because we get here
> > + * outside coroutine context only when executing a command
> > + * out of band, and OOB commands never have QCO_COROUTINE.
> > + */
> > + assert(!oob && qemu_in_coroutine() && !(cmd->options &
> QCO_COROUTINE));
> > +
> > + QmpDispatchBH data = {
> > + .cur_mon = cur_mon,
> > + .cmd = cmd,
> > + .args = args,
> > + .ret = ret,
> > + .errp = errp,
> > + .co = qemu_coroutine_self(),
> > + };
> > + aio_bh_schedule_oneshot(qemu_get_aio_context(),
> do_qmp_dispatch_bh,
> > + &data);
> > + qemu_coroutine_yield();
> > + }
> > +}
> > +
> > /*
> > * Runs outside of coroutine context for OOB commands, but in
> > * coroutine context for everything else.
> > @@ -142,7 +208,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon,
> QObject *req)
> > QDict *error;
> >
> > rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
> > - &mon->common);
> > + qmp_dispatch_run, &mon->common);
> >
> > if (mon->commands == &qmp_cap_negotiation_commands) {
> > error = qdict_get_qdict(rsp, "error");
> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> > index 0990873ec8ec..342b13d7ebbd 100644
> > --- a/qapi/qmp-dispatch.c
> > +++ b/qapi/qmp-dispatch.c
> > @@ -13,7 +13,6 @@
> >
> > #include "qemu/osdep.h"
> >
> > -#include "block/aio.h"
> > #include "qapi/compat-policy.h"
> > #include "qapi/error.h"
> > #include "qapi/qmp/dispatch.h"
> > @@ -22,8 +21,6 @@
> > #include "qapi/qobject-input-visitor.h"
> > #include "qapi/qobject-output-visitor.h"
> > #include "qapi/qmp/qbool.h"
> > -#include "qemu/coroutine.h"
> > -#include "qemu/main-loop.h"
> >
> > Visitor *qobject_input_visitor_new_qmp(QObject *obj)
> > {
> > @@ -110,32 +107,8 @@ bool qmp_is_oob(const QDict *dict)
> > && !qdict_haskey(dict, "execute");
> > }
> >
> > -typedef struct QmpDispatchBH {
> > - const QmpCommand *cmd;
> > - Monitor *cur_mon;
> > - QDict *args;
> > - QObject **ret;
> > - Error **errp;
> > - Coroutine *co;
> > -} QmpDispatchBH;
> > -
> > -static void do_qmp_dispatch_bh(void *opaque)
> > -{
> > - QmpDispatchBH *data = opaque;
> > -
> > - assert(monitor_cur() == NULL);
> > - monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> > - data->cmd->fn(data->args, data->ret, data->errp);
> > - monitor_set_cur(qemu_coroutine_self(), NULL);
> > - aio_co_wake(data->co);
> > -}
> > -
> > -/*
> > - * Runs outside of coroutine context for OOB commands, but in coroutine
> > - * context for everything else.
> > - */
> > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> > - bool allow_oob, Monitor *cur_mon)
> > + bool allow_oob, QmpDispatchRun run_cb, void
> *run_data)
> > {
> > Error *err = NULL;
> > bool oob;
> > @@ -203,39 +176,12 @@ QDict *qmp_dispatch(const QmpCommandList *cmds,
> QObject *request,
> > qobject_ref(args);
> > }
> >
> > - assert(!(oob && qemu_in_coroutine()));
> > - assert(monitor_cur() == NULL);
> > - if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
> > - monitor_set_cur(qemu_coroutine_self(), cur_mon);
> > - cmd->fn(args, &ret, &err);
> > - monitor_set_cur(qemu_coroutine_self(), NULL);
> > + if (run_cb) {
> > + run_cb(oob, cmd, args, &ret, &err, run_data);
> > } else {
> > - /*
> > - * Actual context doesn't match the one the command needs.
> > - *
> > - * Case 1: we are in coroutine context, but command does not
> > - * have QCO_COROUTINE. We need to drop out of coroutine
> > - * context for executing it.
> > - *
> > - * Case 2: we are outside coroutine context, but command has
> > - * QCO_COROUTINE. Can't actually happen, because we get here
> > - * outside coroutine context only when executing a command
> > - * out of band, and OOB commands never have QCO_COROUTINE.
> > - */
> > - assert(!oob && qemu_in_coroutine() && !(cmd->options &
> QCO_COROUTINE));
> > -
> > - QmpDispatchBH data = {
> > - .cur_mon = cur_mon,
> > - .cmd = cmd,
> > - .args = args,
> > - .ret = &ret,
> > - .errp = &err,
> > - .co = qemu_coroutine_self(),
> > - };
> > - aio_bh_schedule_oneshot(qemu_get_aio_context(),
> do_qmp_dispatch_bh,
> > - &data);
> > - qemu_coroutine_yield();
> > + cmd->fn(args, &ret, &err);
> > }
> > +
> > qobject_unref(args);
> > if (err) {
> > /* or assert(!ret) after reviewing all handlers: */
>
> A callback works, but note that each program's function is fixed (the
> simple and common function is inlined, but that's just for convenience).
>
> We could use the linker instead. We already do for
> qmp_command_available(), and the patch doesn't change that.
>
Tbh, using the linker override trick makes me a bit uncomfortable when
trying to make a "common" qemu library.
The "trick" is not well documented (I couldn't find a good reference for
the expected behaviour, and my experience with it isn't great when I
struggled with linking issues earlier). It also makes the library usage a
bit hidden. And it limits the full potential of the library to static
linking.
Callbacks are not always meant to be dynamically changeable.
> Perhaps a layering argument could be made for callbacks. Before the
> series, monitor/qmp.c's monitor_qmp_dispatch() calls
> qapi/qmp-dispatch.c's qmp_dispatch(), which calls a few functions from
> monitor/. However, consistency seems desirable.
>
> What do you think?
>
No strong opinion, as long as the qemu-common project is internal to qemu
projects. If we imagine the code can be made into a shared library, it will
need callbacks.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 14255 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor
2022-08-02 11:19 ` Marc-André Lureau
@ 2022-08-02 12:21 ` Markus Armbruster
0 siblings, 0 replies; 44+ messages in thread
From: Markus Armbruster @ 2022-08-02 12:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Aug 2, 2022 at 3:04 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Make QMP-dispatch code free from QEMU-specific OOB dispatch/async
>> > coroutine handling. This will allow to move the base code to
>> > qemu-common, and clear other users from potential mis-ususe (QGA doesn't
>>
>
> misuse :)
Right :)
>> have OOB or coroutine).
>>
>> I trust the utilty of such a move will become clear later in this
>> series.
>>
>> >
>> > To do that, introduce an optional callback QmpDispatchRun called when a
>> > QMP command should be run, to allow QEMU to override the default
>> > behaviour.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]
>> A callback works, but note that each program's function is fixed (the
>> simple and common function is inlined, but that's just for convenience).
>>
>> We could use the linker instead. We already do for
>> qmp_command_available(), and the patch doesn't change that.
>>
>
> Tbh, using the linker override trick makes me a bit uncomfortable when
> trying to make a "common" qemu library.
This linker behavior goes back to when link archives / libraries were
invented half a century ago. All of stubs/ relies on it.
> The "trick" is not well documented (I couldn't find a good reference for
> the expected behaviour,
I'd recommend John Levine's "Linkers and Loaders". You can find an
archive of the unedited manuscript at
https://archive.ph/20121205032107/http://www.iecc.com/linker/
Chapter 6 applies.
> and my experience with it isn't great when I
> struggled with linking issues earlier). It also makes the library usage a
> bit hidden.
I think the difficulty in understanding shifts.
With link-time resolution, the *possible* resolutions are obvious (by
name), but to see the *actual* resolution, you need to understand how
the program is linked.
With run-time resolution / callbacks, you need to understand run-time
behavior. Can range from obvious to impossible. Your use is certainly
obvious enough.
> And it limits the full potential of the library to static
> linking.
Unix shared libraries make this work, too (they even pay a performance
price for it). For instance, you can override malloc() in a statically
linked .o, and all the .so use it, unless they resort to dark magic to
break this.
> Callbacks are not always meant to be dynamically changeable.
True. See my next paragraph :)
>> Perhaps a layering argument could be made for callbacks. Before the
>> series, monitor/qmp.c's monitor_qmp_dispatch() calls
>> qapi/qmp-dispatch.c's qmp_dispatch(), which calls a few functions from
>> monitor/. However, consistency seems desirable.
>>
>> What do you think?
>>
>
> No strong opinion, as long as the qemu-common project is internal to qemu
> projects. If we imagine the code can be made into a shared library, it will
> need callbacks.
We'll need several more callbacks for that, I'm afraid.
I'd go with link-time resolution for now, simply because that's what we
already use for qmp_command_available() & friends.
I don't like partial replacement by callbacks. I figure a layering
argument could be made for complete replacement.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 07/15] scripts/qapi-gen: add -i option
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (5 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 08/15] scripts/qapi: add required system includes to visitor marcandre.lureau
` (7 subsequent siblings)
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Replace hard-coded "qemu/osdep.h" include with a qapi-gen option to
specify the headers to include. This will allow to substitute QEMU
osdep.h with glib.h for example, for projects with different
global headers.
For historical reasons, we can keep the default as "qemu/osdep.h".
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/commands.py | 15 ++++++++++-----
scripts/qapi/events.py | 17 +++++++++++------
scripts/qapi/gen.py | 17 +++++++++++++++++
scripts/qapi/introspect.py | 11 +++++++----
scripts/qapi/main.py | 17 +++++++++++------
scripts/qapi/types.py | 17 +++++++++++------
scripts/qapi/visit.py | 17 +++++++++++------
7 files changed, 78 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 38ca38a7b9dd..781491b6390d 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -294,9 +294,9 @@ def gen_register_command(name: str,
class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
- def __init__(self, prefix: str, gen_tracing: bool):
+ def __init__(self, prefix: str, include: List[str], gen_tracing: bool):
super().__init__(
- prefix, 'qapi-commands',
+ prefix, include, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', None, __doc__,
gen_tracing=gen_tracing)
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
@@ -308,7 +308,8 @@ def _begin_user_module(self, name: str) -> None:
types = self._module_basename('qapi-types', name)
visit = self._module_basename('qapi-visit', name)
self._genc.add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "qapi/compat-policy.h"
#include "qapi/visitor.h"
#include "qapi/qmp/qdict.h"
@@ -318,6 +319,7 @@ def _begin_user_module(self, name: str) -> None:
#include "%(commands)s.h"
''',
+ include=self.genc_include(),
commands=commands, visit=visit))
if self._gen_tracing and commands != 'qapi-commands':
@@ -344,7 +346,8 @@ def visit_begin(self, schema: QAPISchema) -> None:
''',
c_prefix=c_name(self._prefix, protect=False)))
self._genc.add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "%(prefix)sqapi-commands.h"
#include "%(prefix)sqapi-init-commands.h"
@@ -353,6 +356,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
QTAILQ_INIT(cmds);
''',
+ include=self.genc_include(),
prefix=self._prefix,
c_prefix=c_name(self._prefix, protect=False)))
@@ -404,7 +408,8 @@ def visit_command(self,
def gen_commands(schema: QAPISchema,
output_dir: str,
prefix: str,
+ include: List[str],
gen_tracing: bool) -> None:
- vis = QAPISchemaGenCommandVisitor(prefix, gen_tracing)
+ vis = QAPISchemaGenCommandVisitor(prefix, include, gen_tracing)
schema.visit(vis)
vis.write(output_dir)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 27b44c49f5e9..6e677d11d2e0 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -175,9 +175,9 @@ def gen_event_send(name: str,
class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
- def __init__(self, prefix: str):
+ def __init__(self, prefix: str, include: List[str]):
super().__init__(
- prefix, 'qapi-events',
+ prefix, include, 'qapi-events',
' * Schema-defined QAPI/QMP events', None, __doc__)
self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
self._event_enum_members: List[QAPISchemaEnumMember] = []
@@ -188,7 +188,8 @@ def _begin_user_module(self, name: str) -> None:
types = self._module_basename('qapi-types', name)
visit = self._module_basename('qapi-visit', name)
self._genc.add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "%(prefix)sqapi-emit-events.h"
#include "%(events)s.h"
#include "%(visit)s.h"
@@ -198,6 +199,7 @@ def _begin_user_module(self, name: str) -> None:
#include "qapi/qmp-event.h"
''',
+ include=self.genc_include(),
events=events, visit=visit,
prefix=self._prefix))
self._genh.add(mcgen('''
@@ -209,9 +211,11 @@ def _begin_user_module(self, name: str) -> None:
def visit_end(self) -> None:
self._add_module('./emit', ' * QAPI Events emission')
self._genc.preamble_add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "%(prefix)sqapi-emit-events.h"
''',
+ include=self.genc_include(),
prefix=self._prefix))
self._genh.preamble_add(mcgen('''
#include "qapi/util.h"
@@ -246,7 +250,8 @@ def visit_event(self,
def gen_events(schema: QAPISchema,
output_dir: str,
- prefix: str) -> None:
- vis = QAPISchemaGenEventVisitor(prefix)
+ prefix: str,
+ include: List[str]) -> None:
+ vis = QAPISchemaGenEventVisitor(prefix, include)
schema.visit(vis)
vis.write(output_dir)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 113b49134de4..54a70a5ff516 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -17,6 +17,7 @@
from typing import (
Dict,
Iterator,
+ List,
Optional,
Sequence,
Tuple,
@@ -45,6 +46,12 @@ def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
return ' | '.join(special_features) or '0'
+def genc_include(include: List[str]) -> str:
+ return '\n'.join(['#include ' +
+ (f'"{inc}"' if inc[0] not in ('<', '"') else inc)
+ for inc in include])
+
+
class QAPIGen:
def __init__(self, fname: str):
self.fname = fname
@@ -228,16 +235,21 @@ def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
def __init__(self,
prefix: str,
+ include: List[str],
what: str,
blurb: str,
pydoc: str):
self._prefix = prefix
+ self._include = include
self._what = what
self._genc = QAPIGenC(self._prefix + self._what + '.c',
blurb, pydoc)
self._genh = QAPIGenH(self._prefix + self._what + '.h',
blurb, pydoc)
+ def genc_include(self) -> str:
+ return genc_include(self._include)
+
def write(self, output_dir: str) -> None:
self._genc.write(output_dir)
self._genh.write(output_dir)
@@ -246,12 +258,14 @@ def write(self, output_dir: str) -> None:
class QAPISchemaModularCVisitor(QAPISchemaVisitor):
def __init__(self,
prefix: str,
+ include: List[str],
what: str,
user_blurb: str,
builtin_blurb: Optional[str],
pydoc: str,
gen_tracing: bool = False):
self._prefix = prefix
+ self._include = include
self._what = what
self._user_blurb = user_blurb
self._builtin_blurb = builtin_blurb
@@ -262,6 +276,9 @@ def __init__(self,
self._main_module: Optional[str] = None
self._gen_tracing = gen_tracing
+ def genc_include(self) -> str:
+ return genc_include(self._include)
+
@property
def _genc(self) -> QAPIGenC:
assert self._current_module is not None
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae00..d965d1769447 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -170,9 +170,9 @@ def to_c_string(string: str) -> str:
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
- def __init__(self, prefix: str, unmask: bool):
+ def __init__(self, prefix: str, include: List[str], unmask: bool):
super().__init__(
- prefix, 'qapi-introspect',
+ prefix, include, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
@@ -180,10 +180,12 @@ def __init__(self, prefix: str, unmask: bool):
self._used_types: List[QAPISchemaType] = []
self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "%(prefix)sqapi-introspect.h"
''',
+ include=self.genc_include(),
prefix=prefix))
def visit_begin(self, schema: QAPISchema) -> None:
@@ -384,7 +386,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
+ include: List[str],
opt_unmask: bool) -> None:
- vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
+ vis = QAPISchemaGenIntrospectVisitor(prefix, include, opt_unmask)
schema.visit(vis)
vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index fc216a53d32a..eba98cb9ace2 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -9,7 +9,7 @@
import argparse
import sys
-from typing import Optional
+from typing import List, Optional
from .commands import gen_commands
from .common import must_match
@@ -31,6 +31,7 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
def generate(schema_file: str,
output_dir: str,
prefix: str,
+ include: List[str],
unmask: bool = False,
builtins: bool = False,
gen_tracing: bool = False) -> None:
@@ -48,11 +49,11 @@ def generate(schema_file: str,
assert invalid_prefix_char(prefix) is None
schema = QAPISchema(schema_file)
- gen_types(schema, output_dir, prefix, builtins)
- gen_visit(schema, output_dir, prefix, builtins)
- gen_commands(schema, output_dir, prefix, gen_tracing)
- gen_events(schema, output_dir, prefix)
- gen_introspect(schema, output_dir, prefix, unmask)
+ gen_types(schema, output_dir, prefix, include, builtins)
+ gen_visit(schema, output_dir, prefix, include, builtins)
+ gen_commands(schema, output_dir, prefix, include, gen_tracing)
+ gen_events(schema, output_dir, prefix, include)
+ gen_introspect(schema, output_dir, prefix, include, unmask)
def main() -> int:
@@ -75,6 +76,9 @@ def main() -> int:
parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
dest='unmask',
help="expose non-ABI names in introspection")
+ parser.add_argument('-i', '--include', nargs='*',
+ default=['qemu/osdep.h'],
+ help="top-level include headers")
# Option --suppress-tracing exists so we can avoid solving build system
# problems. TODO Drop it when we no longer need it.
@@ -94,6 +98,7 @@ def main() -> int:
generate(args.schema,
output_dir=args.output_dir,
prefix=args.prefix,
+ include=args.include,
unmask=args.unmask,
builtins=args.builtins,
gen_tracing=not args.suppress_tracing)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 477d02700137..9617b7d4edfa 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -282,18 +282,20 @@ def gen_type_cleanup(name: str) -> str:
class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
- def __init__(self, prefix: str):
+ def __init__(self, prefix: str, include: List[str]):
super().__init__(
- prefix, 'qapi-types', ' * Schema-defined QAPI types',
+ prefix, include, 'qapi-types', ' * Schema-defined QAPI types',
' * Built-in QAPI types', __doc__)
def _begin_builtin_module(self) -> None:
self._genc.preamble_add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "qapi/dealloc-visitor.h"
#include "qapi/qapi-builtin-types.h"
#include "qapi/qapi-builtin-visit.h"
-'''))
+''',
+ include=self.genc_include()))
self._genh.preamble_add(mcgen('''
#include "qapi/util.h"
'''))
@@ -302,11 +304,13 @@ def _begin_user_module(self, name: str) -> None:
types = self._module_basename('qapi-types', name)
visit = self._module_basename('qapi-visit', name)
self._genc.preamble_add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "qapi/dealloc-visitor.h"
#include "%(types)s.h"
#include "%(visit)s.h"
''',
+ include=self.genc_include(),
types=types, visit=visit))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-types.h"
@@ -381,7 +385,8 @@ def visit_alternate_type(self,
def gen_types(schema: QAPISchema,
output_dir: str,
prefix: str,
+ include: List[str],
opt_builtins: bool) -> None:
- vis = QAPISchemaGenTypeVisitor(prefix)
+ vis = QAPISchemaGenTypeVisitor(prefix, include)
schema.visit(vis)
vis.write(output_dir, opt_builtins)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 380fa197f589..1ff464c0360f 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -318,17 +318,19 @@ def gen_visit_object(name: str) -> str:
class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
- def __init__(self, prefix: str):
+ def __init__(self, prefix: str, include: List[str]):
super().__init__(
- prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
+ prefix, include, 'qapi-visit', ' * Schema-defined QAPI visitors',
' * Built-in QAPI visitors', __doc__)
def _begin_builtin_module(self) -> None:
self._genc.preamble_add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "qapi/error.h"
#include "qapi/qapi-builtin-visit.h"
-'''))
+''',
+ include=self.genc_include()))
self._genh.preamble_add(mcgen('''
#include "qapi/visitor.h"
#include "qapi/qapi-builtin-types.h"
@@ -339,11 +341,13 @@ def _begin_user_module(self, name: str) -> None:
types = self._module_basename('qapi-types', name)
visit = self._module_basename('qapi-visit', name)
self._genc.preamble_add(mcgen('''
-#include "qemu/osdep.h"
+%(include)s
+
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "%(visit)s.h"
''',
+ include=self.genc_include(),
visit=visit))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-visit.h"
@@ -408,7 +412,8 @@ def visit_alternate_type(self,
def gen_visit(schema: QAPISchema,
output_dir: str,
prefix: str,
+ include: List[str],
opt_builtins: bool) -> None:
- vis = QAPISchemaGenVisitVisitor(prefix)
+ vis = QAPISchemaGenVisitVisitor(prefix, include)
schema.visit(vis)
vis.write(output_dir, opt_builtins)
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 08/15] scripts/qapi: add required system includes to visitor
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (6 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 07/15] scripts/qapi-gen: add -i option marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 15:08 ` Warner Losh
2022-07-12 9:35 ` [PATCH v2 09/15] util: move 256-by-128 division helpers to int128 marcandre.lureau
` (6 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
The generated visitor code includes abort() & assert(), we shouldn't
rely on the global "-i" headers to include the necessary system headers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/visit.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 1ff464c0360f..4aba5ddd8af4 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -342,6 +342,8 @@ def _begin_user_module(self, name: str) -> None:
visit = self._module_basename('qapi-visit', name)
self._genc.preamble_add(mcgen('''
%(include)s
+#include <assert.h>
+#include <stdlib.h>
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 08/15] scripts/qapi: add required system includes to visitor
2022-07-12 9:35 ` [PATCH v2 08/15] scripts/qapi: add required system includes to visitor marcandre.lureau
@ 2022-07-12 15:08 ` Warner Losh
0 siblings, 0 replies; 44+ messages in thread
From: Warner Losh @ 2022-07-12 15:08 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU Developers, Eric Blake, Cleber Rosa,
open list:Block layer core, Paolo Bonzini, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 980 bytes --]
On Tue, Jul 12, 2022 at 3:36 AM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The generated visitor code includes abort() & assert(), we shouldn't
> rely on the global "-i" headers to include the necessary system headers.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/visit.py | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: Warner Losh <imp@bsdimp.com>
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 1ff464c0360f..4aba5ddd8af4 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -342,6 +342,8 @@ def _begin_user_module(self, name: str) -> None:
> visit = self._module_basename('qapi-visit', name)
> self._genc.preamble_add(mcgen('''
> %(include)s
> +#include <assert.h>
> +#include <stdlib.h>
>
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> --
> 2.37.0.rc0
>
>
[-- Attachment #2: Type: text/html, Size: 1778 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 09/15] util: move 256-by-128 division helpers to int128
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (7 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 08/15] scripts/qapi: add required system includes to visitor marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-08-04 16:17 ` Marc-André Lureau
2022-08-04 17:04 ` Lucas Mateus Martins Araujo e Castro
2022-07-12 9:35 ` [PATCH v2 10/15] qemu-common: introduce a common subproject marcandre.lureau
` (5 subsequent siblings)
14 siblings, 2 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Break a cyclic dependency between int128 and host-utils.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qemu/host-utils.h | 3 -
include/qemu/int128.h | 3 +
util/host-utils.c | 180 --------------------------------------
util/int128.c | 180 ++++++++++++++++++++++++++++++++++++++
4 files changed, 183 insertions(+), 183 deletions(-)
diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
index 29f3a9987880..fa228a4a86e2 100644
--- a/include/qemu/host-utils.h
+++ b/include/qemu/host-utils.h
@@ -32,7 +32,6 @@
#include "qemu/compiler.h"
#include "qemu/bswap.h"
-#include "qemu/int128.h"
#ifdef CONFIG_INT128
static inline void mulu64(uint64_t *plow, uint64_t *phigh,
@@ -785,6 +784,4 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
#endif
}
-Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
-Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
#endif
diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index d2b76ca6acdc..823c61edb0fd 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -472,4 +472,7 @@ static inline void bswap128s(Int128 *s)
#define INT128_MAX int128_make128(UINT64_MAX, INT64_MAX)
#define INT128_MIN int128_make128(0, INT64_MIN)
+Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
+Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
+
#endif /* INT128_H */
diff --git a/util/host-utils.c b/util/host-utils.c
index fb91bcba823d..96d5dc0bed25 100644
--- a/util/host-utils.c
+++ b/util/host-utils.c
@@ -266,183 +266,3 @@ void ulshift(uint64_t *plow, uint64_t *phigh, int32_t shift, bool *overflow)
*plow = *plow << shift;
}
}
-
-/*
- * Unsigned 256-by-128 division.
- * Returns the remainder via r.
- * Returns lower 128 bit of quotient.
- * Needs a normalized divisor (most significant bit set to 1).
- *
- * Adapted from include/qemu/host-utils.h udiv_qrnnd,
- * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
- * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
- *
- * Licensed under the GPLv2/LGPLv3
- */
-static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
-{
- Int128 d0, d1, q0, q1, r1, r0, m;
- uint64_t mp0, mp1;
-
- d0 = int128_make64(int128_getlo(d));
- d1 = int128_make64(int128_gethi(d));
-
- r1 = int128_remu(n1, d1);
- q1 = int128_divu(n1, d1);
- mp0 = int128_getlo(q1);
- mp1 = int128_gethi(q1);
- mulu128(&mp0, &mp1, int128_getlo(d0));
- m = int128_make128(mp0, mp1);
- r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
- if (int128_ult(r1, m)) {
- q1 = int128_sub(q1, int128_one());
- r1 = int128_add(r1, d);
- if (int128_uge(r1, d)) {
- if (int128_ult(r1, m)) {
- q1 = int128_sub(q1, int128_one());
- r1 = int128_add(r1, d);
- }
- }
- }
- r1 = int128_sub(r1, m);
-
- r0 = int128_remu(r1, d1);
- q0 = int128_divu(r1, d1);
- mp0 = int128_getlo(q0);
- mp1 = int128_gethi(q0);
- mulu128(&mp0, &mp1, int128_getlo(d0));
- m = int128_make128(mp0, mp1);
- r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
- if (int128_ult(r0, m)) {
- q0 = int128_sub(q0, int128_one());
- r0 = int128_add(r0, d);
- if (int128_uge(r0, d)) {
- if (int128_ult(r0, m)) {
- q0 = int128_sub(q0, int128_one());
- r0 = int128_add(r0, d);
- }
- }
- }
- r0 = int128_sub(r0, m);
-
- *r = r0;
- return int128_or(int128_lshift(q1, 64), q0);
-}
-
-/*
- * Unsigned 256-by-128 division.
- * Returns the remainder.
- * Returns quotient via plow and phigh.
- * Also returns the remainder via the function return value.
- */
-Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
-{
- Int128 dhi = *phigh;
- Int128 dlo = *plow;
- Int128 rem, dhighest;
- int sh;
-
- if (!int128_nz(divisor) || !int128_nz(dhi)) {
- *plow = int128_divu(dlo, divisor);
- *phigh = int128_zero();
- return int128_remu(dlo, divisor);
- } else {
- sh = clz128(divisor);
-
- if (int128_ult(dhi, divisor)) {
- if (sh != 0) {
- /* normalize the divisor, shifting the dividend accordingly */
- divisor = int128_lshift(divisor, sh);
- dhi = int128_or(int128_lshift(dhi, sh),
- int128_urshift(dlo, (128 - sh)));
- dlo = int128_lshift(dlo, sh);
- }
-
- *phigh = int128_zero();
- *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
- } else {
- if (sh != 0) {
- /* normalize the divisor, shifting the dividend accordingly */
- divisor = int128_lshift(divisor, sh);
- dhighest = int128_rshift(dhi, (128 - sh));
- dhi = int128_or(int128_lshift(dhi, sh),
- int128_urshift(dlo, (128 - sh)));
- dlo = int128_lshift(dlo, sh);
-
- *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
- } else {
- /*
- * dhi >= divisor
- * Since the MSB of divisor is set (sh == 0),
- * (dhi - divisor) < divisor
- *
- * Thus, the high part of the quotient is 1, and we can
- * calculate the low part with a single call to udiv_qrnnd
- * after subtracting divisor from dhi
- */
- dhi = int128_sub(dhi, divisor);
- *phigh = int128_one();
- }
-
- *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
- }
-
- /*
- * since the dividend/divisor might have been normalized,
- * the remainder might also have to be shifted back
- */
- rem = int128_urshift(rem, sh);
- return rem;
- }
-}
-
-/*
- * Signed 256-by-128 division.
- * Returns quotient via plow and phigh.
- * Also returns the remainder via the function return value.
- */
-Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
-{
- bool neg_quotient = false, neg_remainder = false;
- Int128 unsig_hi = *phigh, unsig_lo = *plow;
- Int128 rem;
-
- if (!int128_nonneg(*phigh)) {
- neg_quotient = !neg_quotient;
- neg_remainder = !neg_remainder;
-
- if (!int128_nz(unsig_lo)) {
- unsig_hi = int128_neg(unsig_hi);
- } else {
- unsig_hi = int128_not(unsig_hi);
- unsig_lo = int128_neg(unsig_lo);
- }
- }
-
- if (!int128_nonneg(divisor)) {
- neg_quotient = !neg_quotient;
-
- divisor = int128_neg(divisor);
- }
-
- rem = divu256(&unsig_lo, &unsig_hi, divisor);
-
- if (neg_quotient) {
- if (!int128_nz(unsig_lo)) {
- *phigh = int128_neg(unsig_hi);
- *plow = int128_zero();
- } else {
- *phigh = int128_not(unsig_hi);
- *plow = int128_neg(unsig_lo);
- }
- } else {
- *phigh = unsig_hi;
- *plow = unsig_lo;
- }
-
- if (neg_remainder) {
- return int128_neg(rem);
- } else {
- return rem;
- }
-}
diff --git a/util/int128.c b/util/int128.c
index ed8f25fef161..482c63b6551e 100644
--- a/util/int128.c
+++ b/util/int128.c
@@ -145,3 +145,183 @@ Int128 int128_rems(Int128 a, Int128 b)
}
#endif
+
+/*
+ * Unsigned 256-by-128 division.
+ * Returns the remainder via r.
+ * Returns lower 128 bit of quotient.
+ * Needs a normalized divisor (most significant bit set to 1).
+ *
+ * Adapted from include/qemu/host-utils.h udiv_qrnnd,
+ * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
+ * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
+ *
+ * Licensed under the GPLv2/LGPLv3
+ */
+static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
+{
+ Int128 d0, d1, q0, q1, r1, r0, m;
+ uint64_t mp0, mp1;
+
+ d0 = int128_make64(int128_getlo(d));
+ d1 = int128_make64(int128_gethi(d));
+
+ r1 = int128_remu(n1, d1);
+ q1 = int128_divu(n1, d1);
+ mp0 = int128_getlo(q1);
+ mp1 = int128_gethi(q1);
+ mulu128(&mp0, &mp1, int128_getlo(d0));
+ m = int128_make128(mp0, mp1);
+ r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
+ if (int128_ult(r1, m)) {
+ q1 = int128_sub(q1, int128_one());
+ r1 = int128_add(r1, d);
+ if (int128_uge(r1, d)) {
+ if (int128_ult(r1, m)) {
+ q1 = int128_sub(q1, int128_one());
+ r1 = int128_add(r1, d);
+ }
+ }
+ }
+ r1 = int128_sub(r1, m);
+
+ r0 = int128_remu(r1, d1);
+ q0 = int128_divu(r1, d1);
+ mp0 = int128_getlo(q0);
+ mp1 = int128_gethi(q0);
+ mulu128(&mp0, &mp1, int128_getlo(d0));
+ m = int128_make128(mp0, mp1);
+ r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
+ if (int128_ult(r0, m)) {
+ q0 = int128_sub(q0, int128_one());
+ r0 = int128_add(r0, d);
+ if (int128_uge(r0, d)) {
+ if (int128_ult(r0, m)) {
+ q0 = int128_sub(q0, int128_one());
+ r0 = int128_add(r0, d);
+ }
+ }
+ }
+ r0 = int128_sub(r0, m);
+
+ *r = r0;
+ return int128_or(int128_lshift(q1, 64), q0);
+}
+
+/*
+ * Unsigned 256-by-128 division.
+ * Returns the remainder.
+ * Returns quotient via plow and phigh.
+ * Also returns the remainder via the function return value.
+ */
+Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
+{
+ Int128 dhi = *phigh;
+ Int128 dlo = *plow;
+ Int128 rem, dhighest;
+ int sh;
+
+ if (!int128_nz(divisor) || !int128_nz(dhi)) {
+ *plow = int128_divu(dlo, divisor);
+ *phigh = int128_zero();
+ return int128_remu(dlo, divisor);
+ } else {
+ sh = clz128(divisor);
+
+ if (int128_ult(dhi, divisor)) {
+ if (sh != 0) {
+ /* normalize the divisor, shifting the dividend accordingly */
+ divisor = int128_lshift(divisor, sh);
+ dhi = int128_or(int128_lshift(dhi, sh),
+ int128_urshift(dlo, (128 - sh)));
+ dlo = int128_lshift(dlo, sh);
+ }
+
+ *phigh = int128_zero();
+ *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
+ } else {
+ if (sh != 0) {
+ /* normalize the divisor, shifting the dividend accordingly */
+ divisor = int128_lshift(divisor, sh);
+ dhighest = int128_rshift(dhi, (128 - sh));
+ dhi = int128_or(int128_lshift(dhi, sh),
+ int128_urshift(dlo, (128 - sh)));
+ dlo = int128_lshift(dlo, sh);
+
+ *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
+ } else {
+ /*
+ * dhi >= divisor
+ * Since the MSB of divisor is set (sh == 0),
+ * (dhi - divisor) < divisor
+ *
+ * Thus, the high part of the quotient is 1, and we can
+ * calculate the low part with a single call to udiv_qrnnd
+ * after subtracting divisor from dhi
+ */
+ dhi = int128_sub(dhi, divisor);
+ *phigh = int128_one();
+ }
+
+ *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
+ }
+
+ /*
+ * since the dividend/divisor might have been normalized,
+ * the remainder might also have to be shifted back
+ */
+ rem = int128_urshift(rem, sh);
+ return rem;
+ }
+}
+
+/*
+ * Signed 256-by-128 division.
+ * Returns quotient via plow and phigh.
+ * Also returns the remainder via the function return value.
+ */
+Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
+{
+ bool neg_quotient = false, neg_remainder = false;
+ Int128 unsig_hi = *phigh, unsig_lo = *plow;
+ Int128 rem;
+
+ if (!int128_nonneg(*phigh)) {
+ neg_quotient = !neg_quotient;
+ neg_remainder = !neg_remainder;
+
+ if (!int128_nz(unsig_lo)) {
+ unsig_hi = int128_neg(unsig_hi);
+ } else {
+ unsig_hi = int128_not(unsig_hi);
+ unsig_lo = int128_neg(unsig_lo);
+ }
+ }
+
+ if (!int128_nonneg(divisor)) {
+ neg_quotient = !neg_quotient;
+
+ divisor = int128_neg(divisor);
+ }
+
+ rem = divu256(&unsig_lo, &unsig_hi, divisor);
+
+ if (neg_quotient) {
+ if (!int128_nz(unsig_lo)) {
+ *phigh = int128_neg(unsig_hi);
+ *plow = int128_zero();
+ } else {
+ *phigh = int128_not(unsig_hi);
+ *plow = int128_neg(unsig_lo);
+ }
+ } else {
+ *phigh = unsig_hi;
+ *plow = unsig_lo;
+ }
+
+ if (neg_remainder) {
+ return int128_neg(rem);
+ } else {
+ return rem;
+ }
+}
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 09/15] util: move 256-by-128 division helpers to int128
2022-07-12 9:35 ` [PATCH v2 09/15] util: move 256-by-128 division helpers to int128 marcandre.lureau
@ 2022-08-04 16:17 ` Marc-André Lureau
2022-08-04 17:04 ` Lucas Mateus Martins Araujo e Castro
1 sibling, 0 replies; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-04 16:17 UTC (permalink / raw)
To: qemu-devel, lucas.araujo, Richard Henderson,
Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 14574 bytes --]
Hi
On Tue, Jul 12, 2022 at 1:49 PM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Break a cyclic dependency between int128 and host-utils.
>
Anyone to approve that change? (This will allow us to more easily move
host-utils & int128 units to a subproject)
thanks
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qemu/host-utils.h | 3 -
> include/qemu/int128.h | 3 +
> util/host-utils.c | 180 --------------------------------------
> util/int128.c | 180 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 183 insertions(+), 183 deletions(-)
>
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 29f3a9987880..fa228a4a86e2 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -32,7 +32,6 @@
>
> #include "qemu/compiler.h"
> #include "qemu/bswap.h"
> -#include "qemu/int128.h"
>
> #ifdef CONFIG_INT128
> static inline void mulu64(uint64_t *plow, uint64_t *phigh,
> @@ -785,6 +784,4 @@ static inline uint64_t udiv_qrnnd(uint64_t *r,
> uint64_t n1,
> #endif
> }
>
> -Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
> -Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
> #endif
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index d2b76ca6acdc..823c61edb0fd 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -472,4 +472,7 @@ static inline void bswap128s(Int128 *s)
> #define INT128_MAX int128_make128(UINT64_MAX, INT64_MAX)
> #define INT128_MIN int128_make128(0, INT64_MIN)
>
> +Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
> +Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
> +
> #endif /* INT128_H */
> diff --git a/util/host-utils.c b/util/host-utils.c
> index fb91bcba823d..96d5dc0bed25 100644
> --- a/util/host-utils.c
> +++ b/util/host-utils.c
> @@ -266,183 +266,3 @@ void ulshift(uint64_t *plow, uint64_t *phigh,
> int32_t shift, bool *overflow)
> *plow = *plow << shift;
> }
> }
> -
> -/*
> - * Unsigned 256-by-128 division.
> - * Returns the remainder via r.
> - * Returns lower 128 bit of quotient.
> - * Needs a normalized divisor (most significant bit set to 1).
> - *
> - * Adapted from include/qemu/host-utils.h udiv_qrnnd,
> - * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
> - * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
> - *
> - * Licensed under the GPLv2/LGPLv3
> - */
> -static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
> -{
> - Int128 d0, d1, q0, q1, r1, r0, m;
> - uint64_t mp0, mp1;
> -
> - d0 = int128_make64(int128_getlo(d));
> - d1 = int128_make64(int128_gethi(d));
> -
> - r1 = int128_remu(n1, d1);
> - q1 = int128_divu(n1, d1);
> - mp0 = int128_getlo(q1);
> - mp1 = int128_gethi(q1);
> - mulu128(&mp0, &mp1, int128_getlo(d0));
> - m = int128_make128(mp0, mp1);
> - r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
> - if (int128_ult(r1, m)) {
> - q1 = int128_sub(q1, int128_one());
> - r1 = int128_add(r1, d);
> - if (int128_uge(r1, d)) {
> - if (int128_ult(r1, m)) {
> - q1 = int128_sub(q1, int128_one());
> - r1 = int128_add(r1, d);
> - }
> - }
> - }
> - r1 = int128_sub(r1, m);
> -
> - r0 = int128_remu(r1, d1);
> - q0 = int128_divu(r1, d1);
> - mp0 = int128_getlo(q0);
> - mp1 = int128_gethi(q0);
> - mulu128(&mp0, &mp1, int128_getlo(d0));
> - m = int128_make128(mp0, mp1);
> - r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
> - if (int128_ult(r0, m)) {
> - q0 = int128_sub(q0, int128_one());
> - r0 = int128_add(r0, d);
> - if (int128_uge(r0, d)) {
> - if (int128_ult(r0, m)) {
> - q0 = int128_sub(q0, int128_one());
> - r0 = int128_add(r0, d);
> - }
> - }
> - }
> - r0 = int128_sub(r0, m);
> -
> - *r = r0;
> - return int128_or(int128_lshift(q1, 64), q0);
> -}
> -
> -/*
> - * Unsigned 256-by-128 division.
> - * Returns the remainder.
> - * Returns quotient via plow and phigh.
> - * Also returns the remainder via the function return value.
> - */
> -Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
> -{
> - Int128 dhi = *phigh;
> - Int128 dlo = *plow;
> - Int128 rem, dhighest;
> - int sh;
> -
> - if (!int128_nz(divisor) || !int128_nz(dhi)) {
> - *plow = int128_divu(dlo, divisor);
> - *phigh = int128_zero();
> - return int128_remu(dlo, divisor);
> - } else {
> - sh = clz128(divisor);
> -
> - if (int128_ult(dhi, divisor)) {
> - if (sh != 0) {
> - /* normalize the divisor, shifting the dividend
> accordingly */
> - divisor = int128_lshift(divisor, sh);
> - dhi = int128_or(int128_lshift(dhi, sh),
> - int128_urshift(dlo, (128 - sh)));
> - dlo = int128_lshift(dlo, sh);
> - }
> -
> - *phigh = int128_zero();
> - *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> - } else {
> - if (sh != 0) {
> - /* normalize the divisor, shifting the dividend
> accordingly */
> - divisor = int128_lshift(divisor, sh);
> - dhighest = int128_rshift(dhi, (128 - sh));
> - dhi = int128_or(int128_lshift(dhi, sh),
> - int128_urshift(dlo, (128 - sh)));
> - dlo = int128_lshift(dlo, sh);
> -
> - *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
> - } else {
> - /*
> - * dhi >= divisor
> - * Since the MSB of divisor is set (sh == 0),
> - * (dhi - divisor) < divisor
> - *
> - * Thus, the high part of the quotient is 1, and we can
> - * calculate the low part with a single call to udiv_qrnnd
> - * after subtracting divisor from dhi
> - */
> - dhi = int128_sub(dhi, divisor);
> - *phigh = int128_one();
> - }
> -
> - *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> - }
> -
> - /*
> - * since the dividend/divisor might have been normalized,
> - * the remainder might also have to be shifted back
> - */
> - rem = int128_urshift(rem, sh);
> - return rem;
> - }
> -}
> -
> -/*
> - * Signed 256-by-128 division.
> - * Returns quotient via plow and phigh.
> - * Also returns the remainder via the function return value.
> - */
> -Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
> -{
> - bool neg_quotient = false, neg_remainder = false;
> - Int128 unsig_hi = *phigh, unsig_lo = *plow;
> - Int128 rem;
> -
> - if (!int128_nonneg(*phigh)) {
> - neg_quotient = !neg_quotient;
> - neg_remainder = !neg_remainder;
> -
> - if (!int128_nz(unsig_lo)) {
> - unsig_hi = int128_neg(unsig_hi);
> - } else {
> - unsig_hi = int128_not(unsig_hi);
> - unsig_lo = int128_neg(unsig_lo);
> - }
> - }
> -
> - if (!int128_nonneg(divisor)) {
> - neg_quotient = !neg_quotient;
> -
> - divisor = int128_neg(divisor);
> - }
> -
> - rem = divu256(&unsig_lo, &unsig_hi, divisor);
> -
> - if (neg_quotient) {
> - if (!int128_nz(unsig_lo)) {
> - *phigh = int128_neg(unsig_hi);
> - *plow = int128_zero();
> - } else {
> - *phigh = int128_not(unsig_hi);
> - *plow = int128_neg(unsig_lo);
> - }
> - } else {
> - *phigh = unsig_hi;
> - *plow = unsig_lo;
> - }
> -
> - if (neg_remainder) {
> - return int128_neg(rem);
> - } else {
> - return rem;
> - }
> -}
> diff --git a/util/int128.c b/util/int128.c
> index ed8f25fef161..482c63b6551e 100644
> --- a/util/int128.c
> +++ b/util/int128.c
> @@ -145,3 +145,183 @@ Int128 int128_rems(Int128 a, Int128 b)
> }
>
> #endif
> +
> +/*
> + * Unsigned 256-by-128 division.
> + * Returns the remainder via r.
> + * Returns lower 128 bit of quotient.
> + * Needs a normalized divisor (most significant bit set to 1).
> + *
> + * Adapted from include/qemu/host-utils.h udiv_qrnnd,
> + * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
> + * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
> + *
> + * Licensed under the GPLv2/LGPLv3
> + */
> +static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
> +{
> + Int128 d0, d1, q0, q1, r1, r0, m;
> + uint64_t mp0, mp1;
> +
> + d0 = int128_make64(int128_getlo(d));
> + d1 = int128_make64(int128_gethi(d));
> +
> + r1 = int128_remu(n1, d1);
> + q1 = int128_divu(n1, d1);
> + mp0 = int128_getlo(q1);
> + mp1 = int128_gethi(q1);
> + mulu128(&mp0, &mp1, int128_getlo(d0));
> + m = int128_make128(mp0, mp1);
> + r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
> + if (int128_ult(r1, m)) {
> + q1 = int128_sub(q1, int128_one());
> + r1 = int128_add(r1, d);
> + if (int128_uge(r1, d)) {
> + if (int128_ult(r1, m)) {
> + q1 = int128_sub(q1, int128_one());
> + r1 = int128_add(r1, d);
> + }
> + }
> + }
> + r1 = int128_sub(r1, m);
> +
> + r0 = int128_remu(r1, d1);
> + q0 = int128_divu(r1, d1);
> + mp0 = int128_getlo(q0);
> + mp1 = int128_gethi(q0);
> + mulu128(&mp0, &mp1, int128_getlo(d0));
> + m = int128_make128(mp0, mp1);
> + r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
> + if (int128_ult(r0, m)) {
> + q0 = int128_sub(q0, int128_one());
> + r0 = int128_add(r0, d);
> + if (int128_uge(r0, d)) {
> + if (int128_ult(r0, m)) {
> + q0 = int128_sub(q0, int128_one());
> + r0 = int128_add(r0, d);
> + }
> + }
> + }
> + r0 = int128_sub(r0, m);
> +
> + *r = r0;
> + return int128_or(int128_lshift(q1, 64), q0);
> +}
> +
> +/*
> + * Unsigned 256-by-128 division.
> + * Returns the remainder.
> + * Returns quotient via plow and phigh.
> + * Also returns the remainder via the function return value.
> + */
> +Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
> +{
> + Int128 dhi = *phigh;
> + Int128 dlo = *plow;
> + Int128 rem, dhighest;
> + int sh;
> +
> + if (!int128_nz(divisor) || !int128_nz(dhi)) {
> + *plow = int128_divu(dlo, divisor);
> + *phigh = int128_zero();
> + return int128_remu(dlo, divisor);
> + } else {
> + sh = clz128(divisor);
> +
> + if (int128_ult(dhi, divisor)) {
> + if (sh != 0) {
> + /* normalize the divisor, shifting the dividend
> accordingly */
> + divisor = int128_lshift(divisor, sh);
> + dhi = int128_or(int128_lshift(dhi, sh),
> + int128_urshift(dlo, (128 - sh)));
> + dlo = int128_lshift(dlo, sh);
> + }
> +
> + *phigh = int128_zero();
> + *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> + } else {
> + if (sh != 0) {
> + /* normalize the divisor, shifting the dividend
> accordingly */
> + divisor = int128_lshift(divisor, sh);
> + dhighest = int128_rshift(dhi, (128 - sh));
> + dhi = int128_or(int128_lshift(dhi, sh),
> + int128_urshift(dlo, (128 - sh)));
> + dlo = int128_lshift(dlo, sh);
> +
> + *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
> + } else {
> + /*
> + * dhi >= divisor
> + * Since the MSB of divisor is set (sh == 0),
> + * (dhi - divisor) < divisor
> + *
> + * Thus, the high part of the quotient is 1, and we can
> + * calculate the low part with a single call to udiv_qrnnd
> + * after subtracting divisor from dhi
> + */
> + dhi = int128_sub(dhi, divisor);
> + *phigh = int128_one();
> + }
> +
> + *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> + }
> +
> + /*
> + * since the dividend/divisor might have been normalized,
> + * the remainder might also have to be shifted back
> + */
> + rem = int128_urshift(rem, sh);
> + return rem;
> + }
> +}
> +
> +/*
> + * Signed 256-by-128 division.
> + * Returns quotient via plow and phigh.
> + * Also returns the remainder via the function return value.
> + */
> +Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
> +{
> + bool neg_quotient = false, neg_remainder = false;
> + Int128 unsig_hi = *phigh, unsig_lo = *plow;
> + Int128 rem;
> +
> + if (!int128_nonneg(*phigh)) {
> + neg_quotient = !neg_quotient;
> + neg_remainder = !neg_remainder;
> +
> + if (!int128_nz(unsig_lo)) {
> + unsig_hi = int128_neg(unsig_hi);
> + } else {
> + unsig_hi = int128_not(unsig_hi);
> + unsig_lo = int128_neg(unsig_lo);
> + }
> + }
> +
> + if (!int128_nonneg(divisor)) {
> + neg_quotient = !neg_quotient;
> +
> + divisor = int128_neg(divisor);
> + }
> +
> + rem = divu256(&unsig_lo, &unsig_hi, divisor);
> +
> + if (neg_quotient) {
> + if (!int128_nz(unsig_lo)) {
> + *phigh = int128_neg(unsig_hi);
> + *plow = int128_zero();
> + } else {
> + *phigh = int128_not(unsig_hi);
> + *plow = int128_neg(unsig_lo);
> + }
> + } else {
> + *phigh = unsig_hi;
> + *plow = unsig_lo;
> + }
> +
> + if (neg_remainder) {
> + return int128_neg(rem);
> + } else {
> + return rem;
> + }
> +}
> --
> 2.37.0.rc0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 17646 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 09/15] util: move 256-by-128 division helpers to int128
2022-07-12 9:35 ` [PATCH v2 09/15] util: move 256-by-128 division helpers to int128 marcandre.lureau
2022-08-04 16:17 ` Marc-André Lureau
@ 2022-08-04 17:04 ` Lucas Mateus Martins Araujo e Castro
1 sibling, 0 replies; 44+ messages in thread
From: Lucas Mateus Martins Araujo e Castro @ 2022-08-04 17:04 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 14771 bytes --]
On 12/07/2022 06:35, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
>
> Break a cyclic dependency between int128 and host-utils.
Reviewed-by: Lucas Mateus Castro <lucas.araujo@eldorado.org.br>
>
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>
> ---
> include/qemu/host-utils.h | 3 -
> include/qemu/int128.h | 3 +
> util/host-utils.c | 180 --------------------------------------
> util/int128.c | 180 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 183 insertions(+), 183 deletions(-)
>
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 29f3a9987880..fa228a4a86e2 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -32,7 +32,6 @@
>
> #include "qemu/compiler.h"
> #include "qemu/bswap.h"
> -#include "qemu/int128.h"
>
> #ifdef CONFIG_INT128
> static inline void mulu64(uint64_t *plow, uint64_t *phigh,
> @@ -785,6 +784,4 @@ static inline uint64_t udiv_qrnnd(uint64_t *r, uint64_t n1,
> #endif
> }
>
> -Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
> -Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
> #endif
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index d2b76ca6acdc..823c61edb0fd 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -472,4 +472,7 @@ static inline void bswap128s(Int128 *s)
> #define INT128_MAX int128_make128(UINT64_MAX, INT64_MAX)
> #define INT128_MIN int128_make128(0, INT64_MIN)
>
> +Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor);
> +Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor);
> +
> #endif /* INT128_H */
> diff --git a/util/host-utils.c b/util/host-utils.c
> index fb91bcba823d..96d5dc0bed25 100644
> --- a/util/host-utils.c
> +++ b/util/host-utils.c
> @@ -266,183 +266,3 @@ void ulshift(uint64_t *plow, uint64_t *phigh, int32_t shift, bool *overflow)
> *plow = *plow << shift;
> }
> }
> -
> -/*
> - * Unsigned 256-by-128 division.
> - * Returns the remainder via r.
> - * Returns lower 128 bit of quotient.
> - * Needs a normalized divisor (most significant bit set to 1).
> - *
> - * Adapted from include/qemu/host-utils.h udiv_qrnnd,
> - * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
> - * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
> - *
> - * Licensed under the GPLv2/LGPLv3
> - */
> -static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
> -{
> - Int128 d0, d1, q0, q1, r1, r0, m;
> - uint64_t mp0, mp1;
> -
> - d0 = int128_make64(int128_getlo(d));
> - d1 = int128_make64(int128_gethi(d));
> -
> - r1 = int128_remu(n1, d1);
> - q1 = int128_divu(n1, d1);
> - mp0 = int128_getlo(q1);
> - mp1 = int128_gethi(q1);
> - mulu128(&mp0, &mp1, int128_getlo(d0));
> - m = int128_make128(mp0, mp1);
> - r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
> - if (int128_ult(r1, m)) {
> - q1 = int128_sub(q1, int128_one());
> - r1 = int128_add(r1, d);
> - if (int128_uge(r1, d)) {
> - if (int128_ult(r1, m)) {
> - q1 = int128_sub(q1, int128_one());
> - r1 = int128_add(r1, d);
> - }
> - }
> - }
> - r1 = int128_sub(r1, m);
> -
> - r0 = int128_remu(r1, d1);
> - q0 = int128_divu(r1, d1);
> - mp0 = int128_getlo(q0);
> - mp1 = int128_gethi(q0);
> - mulu128(&mp0, &mp1, int128_getlo(d0));
> - m = int128_make128(mp0, mp1);
> - r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
> - if (int128_ult(r0, m)) {
> - q0 = int128_sub(q0, int128_one());
> - r0 = int128_add(r0, d);
> - if (int128_uge(r0, d)) {
> - if (int128_ult(r0, m)) {
> - q0 = int128_sub(q0, int128_one());
> - r0 = int128_add(r0, d);
> - }
> - }
> - }
> - r0 = int128_sub(r0, m);
> -
> - *r = r0;
> - return int128_or(int128_lshift(q1, 64), q0);
> -}
> -
> -/*
> - * Unsigned 256-by-128 division.
> - * Returns the remainder.
> - * Returns quotient via plow and phigh.
> - * Also returns the remainder via the function return value.
> - */
> -Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
> -{
> - Int128 dhi = *phigh;
> - Int128 dlo = *plow;
> - Int128 rem, dhighest;
> - int sh;
> -
> - if (!int128_nz(divisor) || !int128_nz(dhi)) {
> - *plow = int128_divu(dlo, divisor);
> - *phigh = int128_zero();
> - return int128_remu(dlo, divisor);
> - } else {
> - sh = clz128(divisor);
> -
> - if (int128_ult(dhi, divisor)) {
> - if (sh != 0) {
> - /* normalize the divisor, shifting the dividend accordingly */
> - divisor = int128_lshift(divisor, sh);
> - dhi = int128_or(int128_lshift(dhi, sh),
> - int128_urshift(dlo, (128 - sh)));
> - dlo = int128_lshift(dlo, sh);
> - }
> -
> - *phigh = int128_zero();
> - *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> - } else {
> - if (sh != 0) {
> - /* normalize the divisor, shifting the dividend accordingly */
> - divisor = int128_lshift(divisor, sh);
> - dhighest = int128_rshift(dhi, (128 - sh));
> - dhi = int128_or(int128_lshift(dhi, sh),
> - int128_urshift(dlo, (128 - sh)));
> - dlo = int128_lshift(dlo, sh);
> -
> - *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
> - } else {
> - /*
> - * dhi >= divisor
> - * Since the MSB of divisor is set (sh == 0),
> - * (dhi - divisor) < divisor
> - *
> - * Thus, the high part of the quotient is 1, and we can
> - * calculate the low part with a single call to udiv_qrnnd
> - * after subtracting divisor from dhi
> - */
> - dhi = int128_sub(dhi, divisor);
> - *phigh = int128_one();
> - }
> -
> - *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> - }
> -
> - /*
> - * since the dividend/divisor might have been normalized,
> - * the remainder might also have to be shifted back
> - */
> - rem = int128_urshift(rem, sh);
> - return rem;
> - }
> -}
> -
> -/*
> - * Signed 256-by-128 division.
> - * Returns quotient via plow and phigh.
> - * Also returns the remainder via the function return value.
> - */
> -Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
> -{
> - bool neg_quotient = false, neg_remainder = false;
> - Int128 unsig_hi = *phigh, unsig_lo = *plow;
> - Int128 rem;
> -
> - if (!int128_nonneg(*phigh)) {
> - neg_quotient = !neg_quotient;
> - neg_remainder = !neg_remainder;
> -
> - if (!int128_nz(unsig_lo)) {
> - unsig_hi = int128_neg(unsig_hi);
> - } else {
> - unsig_hi = int128_not(unsig_hi);
> - unsig_lo = int128_neg(unsig_lo);
> - }
> - }
> -
> - if (!int128_nonneg(divisor)) {
> - neg_quotient = !neg_quotient;
> -
> - divisor = int128_neg(divisor);
> - }
> -
> - rem = divu256(&unsig_lo, &unsig_hi, divisor);
> -
> - if (neg_quotient) {
> - if (!int128_nz(unsig_lo)) {
> - *phigh = int128_neg(unsig_hi);
> - *plow = int128_zero();
> - } else {
> - *phigh = int128_not(unsig_hi);
> - *plow = int128_neg(unsig_lo);
> - }
> - } else {
> - *phigh = unsig_hi;
> - *plow = unsig_lo;
> - }
> -
> - if (neg_remainder) {
> - return int128_neg(rem);
> - } else {
> - return rem;
> - }
> -}
> diff --git a/util/int128.c b/util/int128.c
> index ed8f25fef161..482c63b6551e 100644
> --- a/util/int128.c
> +++ b/util/int128.c
> @@ -145,3 +145,183 @@ Int128 int128_rems(Int128 a, Int128 b)
> }
>
> #endif
> +
> +/*
> + * Unsigned 256-by-128 division.
> + * Returns the remainder via r.
> + * Returns lower 128 bit of quotient.
> + * Needs a normalized divisor (most significant bit set to 1).
> + *
> + * Adapted from include/qemu/host-utils.h udiv_qrnnd,
> + * from the GNU Multi Precision Library - longlong.h __udiv_qrnnd
> + * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
> + *
> + * Licensed under the GPLv2/LGPLv3
> + */
> +static Int128 udiv256_qrnnd(Int128 *r, Int128 n1, Int128 n0, Int128 d)
> +{
> + Int128 d0, d1, q0, q1, r1, r0, m;
> + uint64_t mp0, mp1;
> +
> + d0 = int128_make64(int128_getlo(d));
> + d1 = int128_make64(int128_gethi(d));
> +
> + r1 = int128_remu(n1, d1);
> + q1 = int128_divu(n1, d1);
> + mp0 = int128_getlo(q1);
> + mp1 = int128_gethi(q1);
> + mulu128(&mp0, &mp1, int128_getlo(d0));
> + m = int128_make128(mp0, mp1);
> + r1 = int128_make128(int128_gethi(n0), int128_getlo(r1));
> + if (int128_ult(r1, m)) {
> + q1 = int128_sub(q1, int128_one());
> + r1 = int128_add(r1, d);
> + if (int128_uge(r1, d)) {
> + if (int128_ult(r1, m)) {
> + q1 = int128_sub(q1, int128_one());
> + r1 = int128_add(r1, d);
> + }
> + }
> + }
> + r1 = int128_sub(r1, m);
> +
> + r0 = int128_remu(r1, d1);
> + q0 = int128_divu(r1, d1);
> + mp0 = int128_getlo(q0);
> + mp1 = int128_gethi(q0);
> + mulu128(&mp0, &mp1, int128_getlo(d0));
> + m = int128_make128(mp0, mp1);
> + r0 = int128_make128(int128_getlo(n0), int128_getlo(r0));
> + if (int128_ult(r0, m)) {
> + q0 = int128_sub(q0, int128_one());
> + r0 = int128_add(r0, d);
> + if (int128_uge(r0, d)) {
> + if (int128_ult(r0, m)) {
> + q0 = int128_sub(q0, int128_one());
> + r0 = int128_add(r0, d);
> + }
> + }
> + }
> + r0 = int128_sub(r0, m);
> +
> + *r = r0;
> + return int128_or(int128_lshift(q1, 64), q0);
> +}
> +
> +/*
> + * Unsigned 256-by-128 division.
> + * Returns the remainder.
> + * Returns quotient via plow and phigh.
> + * Also returns the remainder via the function return value.
> + */
> +Int128 divu256(Int128 *plow, Int128 *phigh, Int128 divisor)
> +{
> + Int128 dhi = *phigh;
> + Int128 dlo = *plow;
> + Int128 rem, dhighest;
> + int sh;
> +
> + if (!int128_nz(divisor) || !int128_nz(dhi)) {
> + *plow = int128_divu(dlo, divisor);
> + *phigh = int128_zero();
> + return int128_remu(dlo, divisor);
> + } else {
> + sh = clz128(divisor);
> +
> + if (int128_ult(dhi, divisor)) {
> + if (sh != 0) {
> + /* normalize the divisor, shifting the dividend accordingly */
> + divisor = int128_lshift(divisor, sh);
> + dhi = int128_or(int128_lshift(dhi, sh),
> + int128_urshift(dlo, (128 - sh)));
> + dlo = int128_lshift(dlo, sh);
> + }
> +
> + *phigh = int128_zero();
> + *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> + } else {
> + if (sh != 0) {
> + /* normalize the divisor, shifting the dividend accordingly */
> + divisor = int128_lshift(divisor, sh);
> + dhighest = int128_rshift(dhi, (128 - sh));
> + dhi = int128_or(int128_lshift(dhi, sh),
> + int128_urshift(dlo, (128 - sh)));
> + dlo = int128_lshift(dlo, sh);
> +
> + *phigh = udiv256_qrnnd(&dhi, dhighest, dhi, divisor);
> + } else {
> + /*
> + * dhi >= divisor
> + * Since the MSB of divisor is set (sh == 0),
> + * (dhi - divisor) < divisor
> + *
> + * Thus, the high part of the quotient is 1, and we can
> + * calculate the low part with a single call to udiv_qrnnd
> + * after subtracting divisor from dhi
> + */
> + dhi = int128_sub(dhi, divisor);
> + *phigh = int128_one();
> + }
> +
> + *plow = udiv256_qrnnd(&rem, dhi, dlo, divisor);
> + }
> +
> + /*
> + * since the dividend/divisor might have been normalized,
> + * the remainder might also have to be shifted back
> + */
> + rem = int128_urshift(rem, sh);
> + return rem;
> + }
> +}
> +
> +/*
> + * Signed 256-by-128 division.
> + * Returns quotient via plow and phigh.
> + * Also returns the remainder via the function return value.
> + */
> +Int128 divs256(Int128 *plow, Int128 *phigh, Int128 divisor)
> +{
> + bool neg_quotient = false, neg_remainder = false;
> + Int128 unsig_hi = *phigh, unsig_lo = *plow;
> + Int128 rem;
> +
> + if (!int128_nonneg(*phigh)) {
> + neg_quotient = !neg_quotient;
> + neg_remainder = !neg_remainder;
> +
> + if (!int128_nz(unsig_lo)) {
> + unsig_hi = int128_neg(unsig_hi);
> + } else {
> + unsig_hi = int128_not(unsig_hi);
> + unsig_lo = int128_neg(unsig_lo);
> + }
> + }
> +
> + if (!int128_nonneg(divisor)) {
> + neg_quotient = !neg_quotient;
> +
> + divisor = int128_neg(divisor);
> + }
> +
> + rem = divu256(&unsig_lo, &unsig_hi, divisor);
> +
> + if (neg_quotient) {
> + if (!int128_nz(unsig_lo)) {
> + *phigh = int128_neg(unsig_hi);
> + *plow = int128_zero();
> + } else {
> + *phigh = int128_not(unsig_hi);
> + *plow = int128_neg(unsig_lo);
> + }
> + } else {
> + *phigh = unsig_hi;
> + *plow = unsig_lo;
> + }
> +
> + if (neg_remainder) {
> + return int128_neg(rem);
> + } else {
> + return rem;
> + }
> +}
> --
> 2.37.0.rc0
>
>
--
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
[-- Attachment #2: Type: text/html, Size: 15229 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 10/15] qemu-common: introduce a common subproject
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (8 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 09/15] util: move 256-by-128 division helpers to int128 marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 14:57 ` Warner Losh
2022-07-12 9:35 ` [PATCH v2 11/15] qemu-common: move scripts/qapi marcandre.lureau
` (4 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add a new meson subproject to provide common code and scripts for QEMU
and tools. Initially, it will offer QAPI/QMP code generation and
common utilities.
libvhost-user & libvduse will make use of the subproject to avoid having
include/ links to common headers.
The other targeted user is qemu-ga, which will also be converted to a
subproject (so it can be built, moved, released etc independent from QEMU).
Other projects such as qemu-storage-daemon could be built standalone
eventually in the future.
Note that with meson subprojects are "global". Projects will share
subprojects (https://mesonbuild.com/Subprojects.html#subprojects-depending-on-other-subprojects).
We will add extra subprojects/ links to allow standalone subproject
compilation though.
This initial commit simply set the stage to build and link against it.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
meson.build | 9 ++++++++-
.../qemu-common/include}/qemu/help-texts.h | 0
linux-user/meson.build | 4 ++--
subprojects/libvduse/meson.build | 2 ++
subprojects/libvduse/subprojects/qemu-common | 1 +
subprojects/libvhost-user/meson.build | 2 ++
subprojects/libvhost-user/subprojects/qemu-common | 1 +
subprojects/qemu-common/meson.build | 8 ++++++++
8 files changed, 24 insertions(+), 3 deletions(-)
rename {include => subprojects/qemu-common/include}/qemu/help-texts.h (100%)
create mode 120000 subprojects/libvduse/subprojects/qemu-common
create mode 120000 subprojects/libvhost-user/subprojects/qemu-common
create mode 100644 subprojects/qemu-common/meson.build
diff --git a/meson.build b/meson.build
index bc5569ace159..254eb1263a66 100644
--- a/meson.build
+++ b/meson.build
@@ -167,6 +167,10 @@ if 'dtrace' in get_option('trace_backends')
endif
endif
+add_project_arguments('-I' + meson.current_source_dir() / 'subprojects/qemu-common/include',
+ language: ['c', 'cpp', 'objc'],
+)
+
if get_option('iasl') == ''
iasl = find_program('iasl', required: false)
else
@@ -1577,6 +1581,9 @@ if libbpf.found() and not cc.links('''
endif
endif
+qemu_common = subproject('qemu-common')
+qemu_common = qemu_common.get_variable('qemu_common_dep')
+
#################
# config-host.h #
#################
@@ -3052,7 +3059,7 @@ util_ss.add_all(trace_ss)
util_ss = util_ss.apply(config_all, strict: false)
libqemuutil = static_library('qemuutil',
sources: util_ss.sources() + stub_ss.sources() + genh,
- dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman])
+ dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman, qemu_common])
qemuutil = declare_dependency(link_with: libqemuutil,
sources: genh + version_res,
dependencies: [event_loop_base])
diff --git a/include/qemu/help-texts.h b/subprojects/qemu-common/include/qemu/help-texts.h
similarity index 100%
rename from include/qemu/help-texts.h
rename to subprojects/qemu-common/include/qemu/help-texts.h
diff --git a/linux-user/meson.build b/linux-user/meson.build
index de4320af053c..fc6cdb55d657 100644
--- a/linux-user/meson.build
+++ b/linux-user/meson.build
@@ -7,7 +7,7 @@ linux_user_ss = ss.source_set()
common_user_inc += include_directories('include/host/' / host_arch)
common_user_inc += include_directories('include')
-linux_user_ss.add(files(
+linux_user_ss.add([files(
'elfload.c',
'exit.c',
'fd-trans.c',
@@ -20,7 +20,7 @@ linux_user_ss.add(files(
'thunk.c',
'uaccess.c',
'uname.c',
-))
+), qemu_common])
linux_user_ss.add(rt)
linux_user_ss.add(when: 'TARGET_HAS_BFLT', if_true: files('flatload.c'))
diff --git a/subprojects/libvduse/meson.build b/subprojects/libvduse/meson.build
index ba08f5ee1a03..841509ecb996 100644
--- a/subprojects/libvduse/meson.build
+++ b/subprojects/libvduse/meson.build
@@ -2,6 +2,8 @@ project('libvduse', 'c',
license: 'GPL-2.0-or-later',
default_options: ['c_std=gnu99'])
+qemu_common = subproject('qemu-common')
+
libvduse = static_library('vduse',
files('libvduse.c'),
c_args: '-D_GNU_SOURCE')
diff --git a/subprojects/libvduse/subprojects/qemu-common b/subprojects/libvduse/subprojects/qemu-common
new file mode 120000
index 000000000000..4c1c87018a7a
--- /dev/null
+++ b/subprojects/libvduse/subprojects/qemu-common
@@ -0,0 +1 @@
+../../qemu-common
\ No newline at end of file
diff --git a/subprojects/libvhost-user/meson.build b/subprojects/libvhost-user/meson.build
index 39825d9404ae..73355908e072 100644
--- a/subprojects/libvhost-user/meson.build
+++ b/subprojects/libvhost-user/meson.build
@@ -5,6 +5,8 @@ project('libvhost-user', 'c',
threads = dependency('threads')
glib = dependency('glib-2.0')
+qemu_common = subproject('qemu-common')
+
vhost_user = static_library('vhost-user',
files('libvhost-user.c'),
dependencies: threads,
diff --git a/subprojects/libvhost-user/subprojects/qemu-common b/subprojects/libvhost-user/subprojects/qemu-common
new file mode 120000
index 000000000000..4c1c87018a7a
--- /dev/null
+++ b/subprojects/libvhost-user/subprojects/qemu-common
@@ -0,0 +1 @@
+../../qemu-common
\ No newline at end of file
diff --git a/subprojects/qemu-common/meson.build b/subprojects/qemu-common/meson.build
new file mode 100644
index 000000000000..8969b08473ef
--- /dev/null
+++ b/subprojects/qemu-common/meson.build
@@ -0,0 +1,8 @@
+project('qemu-common', 'c',
+ license: 'GPL-2.0-or-later',
+ default_options: ['c_std=gnu11']
+)
+
+qemu_common_dep = declare_dependency(
+ include_directories: include_directories('include'),
+)
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 10/15] qemu-common: introduce a common subproject
2022-07-12 9:35 ` [PATCH v2 10/15] qemu-common: introduce a common subproject marcandre.lureau
@ 2022-07-12 14:57 ` Warner Losh
2022-07-15 11:55 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Warner Losh @ 2022-07-12 14:57 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU Developers, Eric Blake, Cleber Rosa,
open list:Block layer core, Paolo Bonzini, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 6646 bytes --]
On Tue, Jul 12, 2022 at 3:36 AM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Add a new meson subproject to provide common code and scripts for QEMU
> and tools. Initially, it will offer QAPI/QMP code generation and
> common utilities.
>
> libvhost-user & libvduse will make use of the subproject to avoid having
> include/ links to common headers.
>
> The other targeted user is qemu-ga, which will also be converted to a
> subproject (so it can be built, moved, released etc independent from QEMU).
>
> Other projects such as qemu-storage-daemon could be built standalone
> eventually in the future.
>
> Note that with meson subprojects are "global". Projects will share
> subprojects (
> https://mesonbuild.com/Subprojects.html#subprojects-depending-on-other-subprojects
> ).
> We will add extra subprojects/ links to allow standalone subproject
> compilation though.
>
> This initial commit simply set the stage to build and link against it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> meson.build | 9 ++++++++-
> .../qemu-common/include}/qemu/help-texts.h | 0
> linux-user/meson.build | 4 ++--
> subprojects/libvduse/meson.build | 2 ++
> subprojects/libvduse/subprojects/qemu-common | 1 +
> subprojects/libvhost-user/meson.build | 2 ++
> subprojects/libvhost-user/subprojects/qemu-common | 1 +
> subprojects/qemu-common/meson.build | 8 ++++++++
> 8 files changed, 24 insertions(+), 3 deletions(-)
> rename {include => subprojects/qemu-common/include}/qemu/help-texts.h
> (100%)
> create mode 120000 subprojects/libvduse/subprojects/qemu-common
> create mode 120000 subprojects/libvhost-user/subprojects/qemu-common
> create mode 100644 subprojects/qemu-common/meson.build
>
> diff --git a/meson.build b/meson.build
> index bc5569ace159..254eb1263a66 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -167,6 +167,10 @@ if 'dtrace' in get_option('trace_backends')
> endif
> endif
>
> +add_project_arguments('-I' + meson.current_source_dir() /
> 'subprojects/qemu-common/include',
> + language: ['c', 'cpp', 'objc'],
> +)
> +
> if get_option('iasl') == ''
> iasl = find_program('iasl', required: false)
> else
> @@ -1577,6 +1581,9 @@ if libbpf.found() and not cc.links('''
> endif
> endif
>
> +qemu_common = subproject('qemu-common')
> +qemu_common = qemu_common.get_variable('qemu_common_dep')
> +
> #################
> # config-host.h #
> #################
> @@ -3052,7 +3059,7 @@ util_ss.add_all(trace_ss)
> util_ss = util_ss.apply(config_all, strict: false)
> libqemuutil = static_library('qemuutil',
> sources: util_ss.sources() +
> stub_ss.sources() + genh,
> - dependencies: [util_ss.dependencies(), libm,
> threads, glib, socket, malloc, pixman])
> + dependencies: [util_ss.dependencies(), libm,
> threads, glib, socket, malloc, pixman, qemu_common])
> qemuutil = declare_dependency(link_with: libqemuutil,
> sources: genh + version_res,
> dependencies: [event_loop_base])
> diff --git a/include/qemu/help-texts.h
> b/subprojects/qemu-common/include/qemu/help-texts.h
> similarity index 100%
> rename from include/qemu/help-texts.h
> rename to subprojects/qemu-common/include/qemu/help-texts.h
> diff --git a/linux-user/meson.build b/linux-user/meson.build
> index de4320af053c..fc6cdb55d657 100644
> --- a/linux-user/meson.build
> +++ b/linux-user/meson.build
> @@ -7,7 +7,7 @@ linux_user_ss = ss.source_set()
> common_user_inc += include_directories('include/host/' / host_arch)
> common_user_inc += include_directories('include')
>
> -linux_user_ss.add(files(
> +linux_user_ss.add([files(
> 'elfload.c',
> 'exit.c',
> 'fd-trans.c',
> @@ -20,7 +20,7 @@ linux_user_ss.add(files(
> 'thunk.c',
> 'uaccess.c',
> 'uname.c',
> -))
> +), qemu_common])
>
Question: Why does linux-user need these, but bsd-user does not?
Warner
> linux_user_ss.add(rt)
>
> linux_user_ss.add(when: 'TARGET_HAS_BFLT', if_true: files('flatload.c'))
> diff --git a/subprojects/libvduse/meson.build
> b/subprojects/libvduse/meson.build
> index ba08f5ee1a03..841509ecb996 100644
> --- a/subprojects/libvduse/meson.build
> +++ b/subprojects/libvduse/meson.build
> @@ -2,6 +2,8 @@ project('libvduse', 'c',
> license: 'GPL-2.0-or-later',
> default_options: ['c_std=gnu99'])
>
> +qemu_common = subproject('qemu-common')
> +
> libvduse = static_library('vduse',
> files('libvduse.c'),
> c_args: '-D_GNU_SOURCE')
> diff --git a/subprojects/libvduse/subprojects/qemu-common
> b/subprojects/libvduse/subprojects/qemu-common
> new file mode 120000
> index 000000000000..4c1c87018a7a
> --- /dev/null
> +++ b/subprojects/libvduse/subprojects/qemu-common
> @@ -0,0 +1 @@
> +../../qemu-common
> \ No newline at end of file
> diff --git a/subprojects/libvhost-user/meson.build
> b/subprojects/libvhost-user/meson.build
> index 39825d9404ae..73355908e072 100644
> --- a/subprojects/libvhost-user/meson.build
> +++ b/subprojects/libvhost-user/meson.build
> @@ -5,6 +5,8 @@ project('libvhost-user', 'c',
> threads = dependency('threads')
> glib = dependency('glib-2.0')
>
> +qemu_common = subproject('qemu-common')
> +
> vhost_user = static_library('vhost-user',
> files('libvhost-user.c'),
> dependencies: threads,
> diff --git a/subprojects/libvhost-user/subprojects/qemu-common
> b/subprojects/libvhost-user/subprojects/qemu-common
> new file mode 120000
> index 000000000000..4c1c87018a7a
> --- /dev/null
> +++ b/subprojects/libvhost-user/subprojects/qemu-common
> @@ -0,0 +1 @@
> +../../qemu-common
> \ No newline at end of file
> diff --git a/subprojects/qemu-common/meson.build
> b/subprojects/qemu-common/meson.build
> new file mode 100644
> index 000000000000..8969b08473ef
> --- /dev/null
> +++ b/subprojects/qemu-common/meson.build
> @@ -0,0 +1,8 @@
> +project('qemu-common', 'c',
> + license: 'GPL-2.0-or-later',
> + default_options: ['c_std=gnu11']
> +)
> +
> +qemu_common_dep = declare_dependency(
> + include_directories: include_directories('include'),
> +)
> --
> 2.37.0.rc0
>
>
[-- Attachment #2: Type: text/html, Size: 8366 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 10/15] qemu-common: introduce a common subproject
2022-07-12 14:57 ` Warner Losh
@ 2022-07-15 11:55 ` Marc-André Lureau
0 siblings, 0 replies; 44+ messages in thread
From: Marc-André Lureau @ 2022-07-15 11:55 UTC (permalink / raw)
To: Warner Losh
Cc: QEMU Developers, Eric Blake, Cleber Rosa,
open list:Block layer core, Paolo Bonzini, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
Hi
On Tue, Jul 12, 2022 at 6:58 PM Warner Losh <imp@bsdimp.com> wrote:
>
>
>
> On Tue, Jul 12, 2022 at 3:36 AM <marcandre.lureau@redhat.com> wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Add a new meson subproject to provide common code and scripts for QEMU
>> and tools. Initially, it will offer QAPI/QMP code generation and
>> common utilities.
>>
>> libvhost-user & libvduse will make use of the subproject to avoid having
>> include/ links to common headers.
>>
>> The other targeted user is qemu-ga, which will also be converted to a
>> subproject (so it can be built, moved, released etc independent from QEMU).
>>
>> Other projects such as qemu-storage-daemon could be built standalone
>> eventually in the future.
>>
>> Note that with meson subprojects are "global". Projects will share
>> subprojects (https://mesonbuild.com/Subprojects.html#subprojects-depending-on-other-subprojects).
>> We will add extra subprojects/ links to allow standalone subproject
>> compilation though.
>>
>> This initial commit simply set the stage to build and link against it.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> meson.build | 9 ++++++++-
>> .../qemu-common/include}/qemu/help-texts.h | 0
>> linux-user/meson.build | 4 ++--
>> subprojects/libvduse/meson.build | 2 ++
>> subprojects/libvduse/subprojects/qemu-common | 1 +
>> subprojects/libvhost-user/meson.build | 2 ++
>> subprojects/libvhost-user/subprojects/qemu-common | 1 +
>> subprojects/qemu-common/meson.build | 8 ++++++++
>> 8 files changed, 24 insertions(+), 3 deletions(-)
>> rename {include => subprojects/qemu-common/include}/qemu/help-texts.h (100%)
>> create mode 120000 subprojects/libvduse/subprojects/qemu-common
>> create mode 120000 subprojects/libvhost-user/subprojects/qemu-common
>> create mode 100644 subprojects/qemu-common/meson.build
>>
>> diff --git a/meson.build b/meson.build
>> index bc5569ace159..254eb1263a66 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -167,6 +167,10 @@ if 'dtrace' in get_option('trace_backends')
>> endif
>> endif
>>
>> +add_project_arguments('-I' + meson.current_source_dir() / 'subprojects/qemu-common/include',
>> + language: ['c', 'cpp', 'objc'],
>> +)
>> +
>> if get_option('iasl') == ''
>> iasl = find_program('iasl', required: false)
>> else
>> @@ -1577,6 +1581,9 @@ if libbpf.found() and not cc.links('''
>> endif
>> endif
>>
>> +qemu_common = subproject('qemu-common')
>> +qemu_common = qemu_common.get_variable('qemu_common_dep')
>> +
>> #################
>> # config-host.h #
>> #################
>> @@ -3052,7 +3059,7 @@ util_ss.add_all(trace_ss)
>> util_ss = util_ss.apply(config_all, strict: false)
>> libqemuutil = static_library('qemuutil',
>> sources: util_ss.sources() + stub_ss.sources() + genh,
>> - dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman])
>> + dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman, qemu_common])
>> qemuutil = declare_dependency(link_with: libqemuutil,
>> sources: genh + version_res,
>> dependencies: [event_loop_base])
>> diff --git a/include/qemu/help-texts.h b/subprojects/qemu-common/include/qemu/help-texts.h
>> similarity index 100%
>> rename from include/qemu/help-texts.h
>> rename to subprojects/qemu-common/include/qemu/help-texts.h
>> diff --git a/linux-user/meson.build b/linux-user/meson.build
>> index de4320af053c..fc6cdb55d657 100644
>> --- a/linux-user/meson.build
>> +++ b/linux-user/meson.build
>> @@ -7,7 +7,7 @@ linux_user_ss = ss.source_set()
>> common_user_inc += include_directories('include/host/' / host_arch)
>> common_user_inc += include_directories('include')
>>
>> -linux_user_ss.add(files(
>> +linux_user_ss.add([files(
>> 'elfload.c',
>> 'exit.c',
>> 'fd-trans.c',
>> @@ -20,7 +20,7 @@ linux_user_ss.add(files(
>> 'thunk.c',
>> 'uaccess.c',
>> 'uname.c',
>> -))
>> +), qemu_common])
>
>
> Question: Why does linux-user need these, but bsd-user does not?
>
Indeed, it's not needed anymore, thanks!
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (9 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 10/15] qemu-common: introduce a common subproject marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-08-05 8:02 ` Markus Armbruster
2022-07-12 9:35 ` [PATCH v2 12/15] qemu-common: move glib-compat.h marcandre.lureau
` (3 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This is just moving qapi-gen.py and related subdir to qemu-common, to
ease review and proceed step by step. The following patches will move
related necessary code, tests etc.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
docs/conf.py | 2 +-
meson.build | 19 ++-----------------
MAINTAINERS | 4 ++--
subprojects/qemu-common/meson.build | 2 ++
subprojects/qemu-common/scripts/meson.build | 3 +++
.../qemu-common/scripts}/qapi-gen.py | 0
.../qemu-common/scripts}/qapi/.flake8 | 0
.../qemu-common/scripts}/qapi/.isort.cfg | 0
.../qemu-common/scripts}/qapi/__init__.py | 0
.../qemu-common/scripts}/qapi/commands.py | 0
.../qemu-common/scripts}/qapi/common.py | 0
.../qemu-common/scripts}/qapi/error.py | 0
.../qemu-common/scripts}/qapi/events.py | 0
.../qemu-common/scripts}/qapi/expr.py | 0
.../qemu-common/scripts}/qapi/gen.py | 0
.../qemu-common/scripts}/qapi/introspect.py | 0
.../qemu-common/scripts}/qapi/main.py | 0
.../qemu-common/scripts/qapi/meson.build | 16 ++++++++++++++++
.../qemu-common/scripts}/qapi/mypy.ini | 0
.../qemu-common/scripts}/qapi/parser.py | 0
.../qemu-common/scripts}/qapi/pylintrc | 0
.../qemu-common/scripts}/qapi/schema.py | 0
.../qemu-common/scripts}/qapi/source.py | 0
.../qemu-common/scripts}/qapi/types.py | 0
.../qemu-common/scripts}/qapi/visit.py | 0
tests/qapi-schema/meson.build | 2 +-
26 files changed, 27 insertions(+), 21 deletions(-)
create mode 100644 subprojects/qemu-common/scripts/meson.build
rename {scripts => subprojects/qemu-common/scripts}/qapi-gen.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/.flake8 (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/.isort.cfg (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/__init__.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/commands.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/common.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/error.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/events.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/expr.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/gen.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/introspect.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/main.py (100%)
create mode 100644 subprojects/qemu-common/scripts/qapi/meson.build
rename {scripts => subprojects/qemu-common/scripts}/qapi/mypy.ini (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/parser.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/pylintrc (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/schema.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/source.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/types.py (100%)
rename {scripts => subprojects/qemu-common/scripts}/qapi/visit.py (100%)
diff --git a/docs/conf.py b/docs/conf.py
index e33cf3d38121..02dcd987b426 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -56,7 +56,7 @@
# Our extensions are in docs/sphinx; the qapidoc extension requires
# the QAPI modules from scripts/.
sys.path.insert(0, os.path.join(qemu_docdir, "sphinx"))
-sys.path.insert(0, os.path.join(qemu_docdir, "../scripts"))
+sys.path.insert(0, os.path.join(qemu_docdir, "../subprojects/qemu-common/scripts"))
# -- General configuration ------------------------------------------------
diff --git a/meson.build b/meson.build
index 254eb1263a66..97e8ab37eb08 100644
--- a/meson.build
+++ b/meson.build
@@ -1582,6 +1582,8 @@ if libbpf.found() and not cc.links('''
endif
qemu_common = subproject('qemu-common')
+qapi_gen = qemu_common.get_variable('qapi_gen')
+qapi_gen_depends = qemu_common.get_variable('qapi_gen_depends')
qemu_common = qemu_common.get_variable('qemu_common_dep')
#################
@@ -2790,23 +2792,6 @@ genh += configure_file(output: 'config-host.h', configuration: config_host_data)
hxtool = find_program('scripts/hxtool')
shaderinclude = find_program('scripts/shaderinclude.pl')
-qapi_gen = find_program('scripts/qapi-gen.py')
-qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
- meson.current_source_dir() / 'scripts/qapi/commands.py',
- meson.current_source_dir() / 'scripts/qapi/common.py',
- meson.current_source_dir() / 'scripts/qapi/error.py',
- meson.current_source_dir() / 'scripts/qapi/events.py',
- meson.current_source_dir() / 'scripts/qapi/expr.py',
- meson.current_source_dir() / 'scripts/qapi/gen.py',
- meson.current_source_dir() / 'scripts/qapi/introspect.py',
- meson.current_source_dir() / 'scripts/qapi/parser.py',
- meson.current_source_dir() / 'scripts/qapi/schema.py',
- meson.current_source_dir() / 'scripts/qapi/source.py',
- meson.current_source_dir() / 'scripts/qapi/types.py',
- meson.current_source_dir() / 'scripts/qapi/visit.py',
- meson.current_source_dir() / 'scripts/qapi/common.py',
- meson.current_source_dir() / 'scripts/qapi-gen.py'
-]
tracetool = [
python, files('scripts/tracetool.py'),
diff --git a/MAINTAINERS b/MAINTAINERS
index 450abd025271..b45511dab550 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2847,8 +2847,8 @@ F: tests/unit/test-*-visitor.c
F: tests/unit/test-qapi-*.c
F: tests/unit/test-qmp-*.c
F: tests/unit/test-visitor-serialization.c
-F: scripts/qapi-gen.py
-F: scripts/qapi/*
+F: subprojects/qemu-common/scripts/qapi-gen.py
+F: subprojects/qemu-common/scripts/qapi/*
F: docs/sphinx/qapidoc.py
F: docs/devel/qapi*
T: git https://repo.or.cz/qemu/armbru.git qapi-next
diff --git a/subprojects/qemu-common/meson.build b/subprojects/qemu-common/meson.build
index 8969b08473ef..207e53991061 100644
--- a/subprojects/qemu-common/meson.build
+++ b/subprojects/qemu-common/meson.build
@@ -6,3 +6,5 @@ project('qemu-common', 'c',
qemu_common_dep = declare_dependency(
include_directories: include_directories('include'),
)
+
+subdir('scripts')
diff --git a/subprojects/qemu-common/scripts/meson.build b/subprojects/qemu-common/scripts/meson.build
new file mode 100644
index 000000000000..626bed6dcdfe
--- /dev/null
+++ b/subprojects/qemu-common/scripts/meson.build
@@ -0,0 +1,3 @@
+qapi_gen = find_program('qapi-gen.py')
+
+subdir('qapi')
diff --git a/scripts/qapi-gen.py b/subprojects/qemu-common/scripts/qapi-gen.py
similarity index 100%
rename from scripts/qapi-gen.py
rename to subprojects/qemu-common/scripts/qapi-gen.py
diff --git a/scripts/qapi/.flake8 b/subprojects/qemu-common/scripts/qapi/.flake8
similarity index 100%
rename from scripts/qapi/.flake8
rename to subprojects/qemu-common/scripts/qapi/.flake8
diff --git a/scripts/qapi/.isort.cfg b/subprojects/qemu-common/scripts/qapi/.isort.cfg
similarity index 100%
rename from scripts/qapi/.isort.cfg
rename to subprojects/qemu-common/scripts/qapi/.isort.cfg
diff --git a/scripts/qapi/__init__.py b/subprojects/qemu-common/scripts/qapi/__init__.py
similarity index 100%
rename from scripts/qapi/__init__.py
rename to subprojects/qemu-common/scripts/qapi/__init__.py
diff --git a/scripts/qapi/commands.py b/subprojects/qemu-common/scripts/qapi/commands.py
similarity index 100%
rename from scripts/qapi/commands.py
rename to subprojects/qemu-common/scripts/qapi/commands.py
diff --git a/scripts/qapi/common.py b/subprojects/qemu-common/scripts/qapi/common.py
similarity index 100%
rename from scripts/qapi/common.py
rename to subprojects/qemu-common/scripts/qapi/common.py
diff --git a/scripts/qapi/error.py b/subprojects/qemu-common/scripts/qapi/error.py
similarity index 100%
rename from scripts/qapi/error.py
rename to subprojects/qemu-common/scripts/qapi/error.py
diff --git a/scripts/qapi/events.py b/subprojects/qemu-common/scripts/qapi/events.py
similarity index 100%
rename from scripts/qapi/events.py
rename to subprojects/qemu-common/scripts/qapi/events.py
diff --git a/scripts/qapi/expr.py b/subprojects/qemu-common/scripts/qapi/expr.py
similarity index 100%
rename from scripts/qapi/expr.py
rename to subprojects/qemu-common/scripts/qapi/expr.py
diff --git a/scripts/qapi/gen.py b/subprojects/qemu-common/scripts/qapi/gen.py
similarity index 100%
rename from scripts/qapi/gen.py
rename to subprojects/qemu-common/scripts/qapi/gen.py
diff --git a/scripts/qapi/introspect.py b/subprojects/qemu-common/scripts/qapi/introspect.py
similarity index 100%
rename from scripts/qapi/introspect.py
rename to subprojects/qemu-common/scripts/qapi/introspect.py
diff --git a/scripts/qapi/main.py b/subprojects/qemu-common/scripts/qapi/main.py
similarity index 100%
rename from scripts/qapi/main.py
rename to subprojects/qemu-common/scripts/qapi/main.py
diff --git a/subprojects/qemu-common/scripts/qapi/meson.build b/subprojects/qemu-common/scripts/qapi/meson.build
new file mode 100644
index 000000000000..5f73a966f344
--- /dev/null
+++ b/subprojects/qemu-common/scripts/qapi/meson.build
@@ -0,0 +1,16 @@
+qapi_gen_depends = files(
+ '__init__.py',
+ 'commands.py',
+ 'common.py',
+ 'error.py',
+ 'events.py',
+ 'expr.py',
+ 'gen.py',
+ 'introspect.py',
+ 'parser.py',
+ 'schema.py',
+ 'source.py',
+ 'types.py',
+ 'visit.py',
+ 'common.py',
+)
diff --git a/scripts/qapi/mypy.ini b/subprojects/qemu-common/scripts/qapi/mypy.ini
similarity index 100%
rename from scripts/qapi/mypy.ini
rename to subprojects/qemu-common/scripts/qapi/mypy.ini
diff --git a/scripts/qapi/parser.py b/subprojects/qemu-common/scripts/qapi/parser.py
similarity index 100%
rename from scripts/qapi/parser.py
rename to subprojects/qemu-common/scripts/qapi/parser.py
diff --git a/scripts/qapi/pylintrc b/subprojects/qemu-common/scripts/qapi/pylintrc
similarity index 100%
rename from scripts/qapi/pylintrc
rename to subprojects/qemu-common/scripts/qapi/pylintrc
diff --git a/scripts/qapi/schema.py b/subprojects/qemu-common/scripts/qapi/schema.py
similarity index 100%
rename from scripts/qapi/schema.py
rename to subprojects/qemu-common/scripts/qapi/schema.py
diff --git a/scripts/qapi/source.py b/subprojects/qemu-common/scripts/qapi/source.py
similarity index 100%
rename from scripts/qapi/source.py
rename to subprojects/qemu-common/scripts/qapi/source.py
diff --git a/scripts/qapi/types.py b/subprojects/qemu-common/scripts/qapi/types.py
similarity index 100%
rename from scripts/qapi/types.py
rename to subprojects/qemu-common/scripts/qapi/types.py
diff --git a/scripts/qapi/visit.py b/subprojects/qemu-common/scripts/qapi/visit.py
similarity index 100%
rename from scripts/qapi/visit.py
rename to subprojects/qemu-common/scripts/qapi/visit.py
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index c18dd7d02f72..2c676360041e 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -1,5 +1,5 @@
test_env = environment()
-test_env.set('PYTHONPATH', meson.project_source_root() / 'scripts')
+test_env.set('PYTHONPATH', meson.project_source_root() / 'subprojects/qemu-common/scripts')
test_env.set('PYTHONIOENCODING', 'utf-8')
schemas = [
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-07-12 9:35 ` [PATCH v2 11/15] qemu-common: move scripts/qapi marcandre.lureau
@ 2022-08-05 8:02 ` Markus Armbruster
2022-08-05 8:49 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-08-05 8:02 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This is just moving qapi-gen.py and related subdir to qemu-common, to
> ease review and proceed step by step. The following patches will move
> related necessary code, tests etc.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
As moved files tend to become low-level annoyances for a long time, I'd
like to understand why you want to move them. The commit message says
"to ease review", which I suspect isn't the real reason. Perhaps you
explained all that elsewhere already, but I missed it.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-05 8:02 ` Markus Armbruster
@ 2022-08-05 8:49 ` Marc-André Lureau
2022-08-11 6:52 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-05 8:49 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]
Hi
On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This is just moving qapi-gen.py and related subdir to qemu-common, to
> > ease review and proceed step by step. The following patches will move
> > related necessary code, tests etc.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> As moved files tend to become low-level annoyances for a long time, I'd
> like to understand why you want to move them. The commit message says
> "to ease review", which I suspect isn't the real reason. Perhaps you
> explained all that elsewhere already, but I missed it.
>
>
>
The end goal is to split some projects, such as qemu-ga, to standalone
meson projects/subprojects. We will be able to build them independently
from the rest of QEMU, and later on perhaps handle them outside of QEMU
main repository. To achieve this, I first introduce a qemu-common
subproject, where qapi and common units are provided. You can check
https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek at
current end result.
I said "to ease review and proceed step by step" simply because there are
no other changes: I don't move the rest of the qapi code & tests all
together, it's in the subsequent series.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2181 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-05 8:49 ` Marc-André Lureau
@ 2022-08-11 6:52 ` Markus Armbruster
2022-08-11 7:11 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-08-11 6:52 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > This is just moving qapi-gen.py and related subdir to qemu-common, to
>> > ease review and proceed step by step. The following patches will move
>> > related necessary code, tests etc.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> As moved files tend to become low-level annoyances for a long time, I'd
>> like to understand why you want to move them. The commit message says
>> "to ease review", which I suspect isn't the real reason. Perhaps you
>> explained all that elsewhere already, but I missed it.
>>
>>
>>
> The end goal is to split some projects, such as qemu-ga, to standalone
> meson projects/subprojects. We will be able to build them independently
> from the rest of QEMU, and later on perhaps handle them outside of QEMU
> main repository. To achieve this, I first introduce a qemu-common
> subproject, where qapi and common units are provided. You can check
> https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek at
> current end result.
I worry this move of the QAPI generator code into
subjprojects/common/scripts/qapi/ will be followed by a move into its
own subproject.
Ignorant question: could we turn the QAPI generator into a subproject in
place?
> I said "to ease review and proceed step by step" simply because there are
> no other changes: I don't move the rest of the qapi code & tests all
> together, it's in the subsequent series.
I'd recommend to provide a bit more context in the commit message, even
if you copy it to several messages in a row. Our future selves will
likely be grateful.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 6:52 ` Markus Armbruster
@ 2022-08-11 7:11 ` Marc-André Lureau
2022-08-11 9:05 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-11 7:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]
Hi
On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster <armbru@redhat.com>
wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > This is just moving qapi-gen.py and related subdir to qemu-common, to
> >> > ease review and proceed step by step. The following patches will move
> >> > related necessary code, tests etc.
> >> >
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> As moved files tend to become low-level annoyances for a long time, I'd
> >> like to understand why you want to move them. The commit message says
> >> "to ease review", which I suspect isn't the real reason. Perhaps you
> >> explained all that elsewhere already, but I missed it.
> >>
> >>
> >>
> > The end goal is to split some projects, such as qemu-ga, to standalone
> > meson projects/subprojects. We will be able to build them independently
> > from the rest of QEMU, and later on perhaps handle them outside of QEMU
> > main repository. To achieve this, I first introduce a qemu-common
> > subproject, where qapi and common units are provided. You can check
> > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek
> at
> > current end result.
>
> I worry this move of the QAPI generator code into
> subjprojects/common/scripts/qapi/ will be followed by a move into its
> own subproject.
>
Do you mean: it could be moved again to another smaller subproject? not
really, see below
> Ignorant question: could we turn the QAPI generator into a subproject in
> place?
>
If it's just the generator, probably the target would then be a python
project (not meson), similar to python-qemu-qmp.
But I don't see much point, since it's not really a standalone python
module, it generates code, and that code needs most of what is in
qemu-common (see
https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common).
It's best to have it together imho. Maybe we can consider a different
naming or to be more careful not to add stuff that is not strictly needed
by qapi?
(fwiw, it's a bit of a shame python-qemu-qmp didn't import git history from
qemu.. we did better with libslirp. If we ever move code in standalone
repositories again, we should be careful to keep history with it)
> > I said "to ease review and proceed step by step" simply because there are
> > no other changes: I don't move the rest of the qapi code & tests all
> > together, it's in the subsequent series.
>
> I'd recommend to provide a bit more context in the commit message, even
> if you copy it to several messages in a row. Our future selves will
> likely be grateful.
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4524 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 7:11 ` Marc-André Lureau
@ 2022-08-11 9:05 ` Markus Armbruster
2022-08-11 10:09 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-08-11 9:05 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>> > Hi
>> >
>> > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>> >
>> >> marcandre.lureau@redhat.com writes:
>> >>
>> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> > This is just moving qapi-gen.py and related subdir to qemu-common, to
>> >> > ease review and proceed step by step. The following patches will move
>> >> > related necessary code, tests etc.
>> >> >
>> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >>
>> >> As moved files tend to become low-level annoyances for a long time, I'd
>> >> like to understand why you want to move them. The commit message says
>> >> "to ease review", which I suspect isn't the real reason. Perhaps you
>> >> explained all that elsewhere already, but I missed it.
>> >>
>> >>
>> >>
>> > The end goal is to split some projects, such as qemu-ga, to standalone
>> > meson projects/subprojects. We will be able to build them independently
>> > from the rest of QEMU, and later on perhaps handle them outside of QEMU
>> > main repository. To achieve this, I first introduce a qemu-common
>> > subproject, where qapi and common units are provided. You can check
>> > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek at
>> > current end result.
>>
>> I worry this move of the QAPI generator code into
>> subjprojects/common/scripts/qapi/ will be followed by a move into its
>> own subproject.
>>
>
> Do you mean: it could be moved again to another smaller subproject? not
> really, see below
>
>
>> Ignorant question: could we turn the QAPI generator into a subproject in
>> place?
>>
>
> If it's just the generator, probably the target would then be a python
> project (not meson), similar to python-qemu-qmp.
>
> But I don't see much point, since it's not really a standalone python
> module, it generates code, and that code needs most of what is in
> qemu-common (see
> https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common).
> It's best to have it together imho. Maybe we can consider a different
> naming or to be more careful not to add stuff that is not strictly needed
> by qapi?
I had a look at subjprojects/qemu-common in your qga branch. Contents:
* Subproject machinery
* Some common headers (glib-compat.h), but not others (qemu/osdep.h). I
guess it's whatever subjproject code needs.
Is subprojects/qemu-common/include/block/nvme.h there by accident?
* Most of the QObject subsystem
qobject/block-qdict.c is left behind.
* Most of the QAPI subsystem
Some visitors left behind: opts, forward, string input / output. Hmm,
only the .c, the .h are in the subjproject. Accident?
A bit of HMP support left behind.
* Parts of util/ and include/qemu/
Error reporting, key-value CLI, some C utilities, but not others
(e.g. qemu/atomic.h, but not qemu/atomic128.h). I guess it's again
whatever subjproject code needs.
* Parts of the QAPI Schema subsystem
Aside: MAINTAINERS mostly not updated.
Your moves tear closely related code apart. This is going to be a drag
for developers in general and maintainers in particular.
Ergonomics suffer when related code is in multiple places. Having to
switch between directories and remember where is what will a constant
low-level pain. Things that used to be simple & quick, like git-grep
qapi/*.c, become more involved.
Hurts even when merely consuming the subsystem: when I see #include
"qemu/foo.h", the straightforward include/qemu/foo.h may or may not do.
When it doesn't, I need to know where to look instead.
subprojects/qemu-common/include/ is a lot to type. Sufficiently
powerful editors mitigate, but not completely.
When changes need to be applied to every instance of an abstraction,
it's easy to miss instances "elsewhere". There's a reason the QAPI
visitors are all in one place.
The actual split seems somewhat arbitrary in places. I suspect more
code will move over time. Invalidating "what is where" knowledge.
I believe a serious think about other ways to accomplish your goals is
called for.
> (fwiw, it's a bit of a shame python-qemu-qmp didn't import git history from
> qemu.. we did better with libslirp. If we ever move code in standalone
> repositories again, we should be careful to keep history with it)
Yes, we should preserve history whenever practical.
[...]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 9:05 ` Markus Armbruster
@ 2022-08-11 10:09 ` Marc-André Lureau
2022-08-11 10:22 ` Peter Maydell
0 siblings, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-11 10:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, Peter Maydell, John Snow, Michael Roth,
Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 6442 bytes --]
On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster <armbru@redhat.com>
> > wrote:
> >
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >>
> >> > Hi
> >> >
> >> > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >> >
> >> >> marcandre.lureau@redhat.com writes:
> >> >>
> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >
> >> >> > This is just moving qapi-gen.py and related subdir to qemu-common,
> to
> >> >> > ease review and proceed step by step. The following patches will
> move
> >> >> > related necessary code, tests etc.
> >> >> >
> >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> As moved files tend to become low-level annoyances for a long time,
> I'd
> >> >> like to understand why you want to move them. The commit message
> says
> >> >> "to ease review", which I suspect isn't the real reason. Perhaps you
> >> >> explained all that elsewhere already, but I missed it.
> >> >>
> >> >>
> >> >>
> >> > The end goal is to split some projects, such as qemu-ga, to standalone
> >> > meson projects/subprojects. We will be able to build them
> independently
> >> > from the rest of QEMU, and later on perhaps handle them outside of
> QEMU
> >> > main repository. To achieve this, I first introduce a qemu-common
> >> > subproject, where qapi and common units are provided. You can check
> >> > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak
> peek at
> >> > current end result.
> >>
> >> I worry this move of the QAPI generator code into
> >> subjprojects/common/scripts/qapi/ will be followed by a move into its
> >> own subproject.
> >>
> >
> > Do you mean: it could be moved again to another smaller subproject? not
> > really, see below
> >
> >
> >> Ignorant question: could we turn the QAPI generator into a subproject in
> >> place?
> >>
> >
> > If it's just the generator, probably the target would then be a python
> > project (not meson), similar to python-qemu-qmp.
> >
> > But I don't see much point, since it's not really a standalone python
> > module, it generates code, and that code needs most of what is in
> > qemu-common (see
> >
> https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common
> ).
> > It's best to have it together imho. Maybe we can consider a different
> > naming or to be more careful not to add stuff that is not strictly needed
> > by qapi?
>
> I had a look at subjprojects/qemu-common in your qga branch. Contents:
>
> * Subproject machinery
>
> * Some common headers (glib-compat.h), but not others (qemu/osdep.h). I
> guess it's whatever subjproject code needs.
>
> Is subprojects/qemu-common/include/block/nvme.h there by accident?
>
It's a header shared with qemu-ga (the commit message explains)
>
> * Most of the QObject subsystem
>
> qobject/block-qdict.c is left behind.
>
It's qemu block specific. Not needed to move at this point, it drags other
stuff iirc.
> * Most of the QAPI subsystem
>
> Some visitors left behind: opts, forward, string input / output. Hmm,
> only the .c, the .h are in the subjproject. Accident?
>
If they are not shared with qemu-ga, I didn't move them yet. They can stay
specific to qemu or specific subprojects, or we can decide to move them (if
that doesn't drag too much stuff along).
>
> A bit of HMP support left behind.
>
Can you be more specific?
>
> * Parts of util/ and include/qemu/
>
> Error reporting, key-value CLI, some C utilities, but not others
> (e.g. qemu/atomic.h, but not qemu/atomic128.h). I guess it's again
> whatever subjproject code needs.
>
>
* Parts of the QAPI Schema subsystem
>
> Aside: MAINTAINERS mostly not updated.
>
>
That needs fixing.
> Your moves tear closely related code apart. This is going to be a drag
> for developers in general and maintainers in particular.
>
> Ergonomics suffer when related code is in multiple places. Having to
> switch between directories and remember where is what will a constant
> low-level pain. Things that used to be simple & quick, like git-grep
> qapi/*.c, become more involved.
>
>
It's inevitable when a project grows. QEMU is already a very large project.
Over the years, we have structured the project, by moving things and
splitting in subdirectories. Imho, this is actually helpful in many ways,
and we got used to it easily hopefully.
Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd,
storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be
tight to qemu code, release and management process? I am not saying it's
time to move them out of qemu project, but I believe it's helpful to have
code that is structured and can be compiled indepently.
And by having "standalone" subprojects, we can more easily evolve in new
directions, without touching the rest of the projects.
Hurts even when merely consuming the subsystem: when I see #include
> "qemu/foo.h", the straightforward include/qemu/foo.h may or may not do.
> When it doesn't, I need to know where to look instead.
>
> subprojects/qemu-common/include/ is a lot to type. Sufficiently
> powerful editors mitigate, but not completely.
>
> When changes need to be applied to every instance of an abstraction,
> it's easy to miss instances "elsewhere". There's a reason the QAPI
> visitors are all in one place.
>
I understand it's nice to have all the code in one place. At the same time,
this goes against modularity and composability..
>
> The actual split seems somewhat arbitrary in places. I suspect more
> code will move over time. Invalidating "what is where" knowledge.
>
Are you insinuating that this is obvious today? :) By having smaller
standalone projects, we have better defined scope, test range, reusability
etc. And we avoid creating a jam of dependencies, making code review &
change a bit easier..
> I believe a serious think about other ways to accomplish your goals is
> called for.
>
I don't claim to have the perfect solution. It's evolution, hopefully a
step in a better direction.
thanks
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 9906 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 10:09 ` Marc-André Lureau
@ 2022-08-11 10:22 ` Peter Maydell
2022-08-11 10:50 ` Marc-André Lureau
2022-08-11 11:46 ` Markus Armbruster
0 siblings, 2 replies; 44+ messages in thread
From: Peter Maydell @ 2022-08-11 10:22 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, qemu-devel, Eric Blake, Cleber Rosa,
qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans, John Snow,
Michael Roth, Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
On Thu, 11 Aug 2022 at 11:09, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>> Your moves tear closely related code apart. This is going to be a drag
>> for developers in general and maintainers in particular.
>>
>> Ergonomics suffer when related code is in multiple places. Having to
>> switch between directories and remember where is what will a constant
>> low-level pain. Things that used to be simple & quick, like git-grep
>> qapi/*.c, become more involved.
>>
>
> It's inevitable when a project grows. QEMU is already a very large project. Over the years, we have structured the project, by moving things and splitting in subdirectories. Imho, this is actually helpful in many ways, and we got used to it easily hopefully.
I agree with this. But generally we've tried to do that by
having the subdirectory splitting and naming match the
structure of the codebase. The exception, which I strongly
dislike, is that contrib/ is a grabbag of random stuff.
subprojects/ is starting to feel like it is also turning
into "place where random stuff ends up"...
> Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd, storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be tight to qemu code, release and management process? I am not saying it's time to move them out of qemu project, but I believe it's helpful to have code that is structured and can be compiled indepently.
>
> And by having "standalone" subprojects, we can more easily evolve in new directions, without touching the rest of the projects.
As Markus says, your branch ends up moving most of qobject into
qemu-common/. We are never going to let that out of QEMU proper,
because we are never going to allow ourselves to be tied to API
compatibility with it as an external library. So anything that
needs qobject is never going to leave the QEMU codebase. Which
means that there's not much gain from shoving it into subproject/
IMHO.
If there's stuff that is reasonably independent then it may be
worth disentangling from our build process, but it has to be
actually independent, or made that way, first, I think. And,
as with libslirp, there needs to be a clear beneficiary who
would want to take that independent-sub-thingy and use it
entirely distinctly from QEMU.
thanks
-- PMM
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 10:22 ` Peter Maydell
@ 2022-08-11 10:50 ` Marc-André Lureau
2022-08-11 12:15 ` Daniel P. Berrangé
2022-08-11 11:46 ` Markus Armbruster
1 sibling, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-11 10:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Markus Armbruster, qemu-devel, Eric Blake, Cleber Rosa,
qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans, John Snow,
Michael Roth, Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]
Hi
On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <peter.maydell@linaro.org>
wrote:
> On Thu, 11 Aug 2022 at 11:09, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >> Your moves tear closely related code apart. This is going to be a drag
> >> for developers in general and maintainers in particular.
> >>
> >> Ergonomics suffer when related code is in multiple places. Having to
> >> switch between directories and remember where is what will a constant
> >> low-level pain. Things that used to be simple & quick, like git-grep
> >> qapi/*.c, become more involved.
> >>
> >
> > It's inevitable when a project grows. QEMU is already a very large
> project. Over the years, we have structured the project, by moving things
> and splitting in subdirectories. Imho, this is actually helpful in many
> ways, and we got used to it easily hopefully.
>
> I agree with this. But generally we've tried to do that by
> having the subdirectory splitting and naming match the
> structure of the codebase. The exception, which I strongly
> dislike, is that contrib/ is a grabbag of random stuff.
> subprojects/ is starting to feel like it is also turning
> into "place where random stuff ends up"...
>
Yes, most of contrib/* should probably be standalone projects. If only we
had some kind of common library/subproject :)
subproject/* is a meson *cough* convention (imposed location for
subprojects). If we don't want to depend on external released libraries,
that's just the way it is.
>
> > Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd,
> storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be
> tight to qemu code, release and management process? I am not saying it's
> time to move them out of qemu project, but I believe it's helpful to have
> code that is structured and can be compiled indepently.
> >
> > And by having "standalone" subprojects, we can more easily evolve in new
> directions, without touching the rest of the projects.
>
> As Markus says, your branch ends up moving most of qobject into
> qemu-common/. We are never going to let that out of QEMU proper,
> because we are never going to allow ourselves to be tied to API
> compatibility with it as an external library. So anything that
>
Why is that? We do it with a lot of dependencies already, with stable APIs.
Furthermore, we don't "have to" be tied to a specific ABI/API, we can
continue to link statically and compile against a specific version. like
with libvfio-user today.
And at this point, I am _not_ proposing to have an extra "qemu-common"
repository. I don't think there are enough reasons to want that either.
> needs qobject is never going to leave the QEMU codebase. Which
>
means that there's not much gain from shoving it into subproject/
> IMHO.
(just to be extra clear, it's qobject not QOM we are talking about)
qobject is fundamental to all the QAPI related generated code. Why should
that remain tight to QEMU proper?
thanks
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4561 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 10:50 ` Marc-André Lureau
@ 2022-08-11 12:15 ` Daniel P. Berrangé
2022-08-11 13:35 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Daniel P. Berrangé @ 2022-08-11 12:15 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Peter Maydell, Markus Armbruster, qemu-devel, Eric Blake,
Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans,
John Snow, Michael Roth, Warner Losh, Kevin Wolf,
Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Laurent Vivier, Fam Zheng, Hanna Reitz
On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
> > On Thu, 11 Aug 2022 at 11:09, Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > > On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com>
> > wrote:
> > >> Your moves tear closely related code apart. This is going to be a drag
> > >> for developers in general and maintainers in particular.
> > >>
> > >> Ergonomics suffer when related code is in multiple places. Having to
> > >> switch between directories and remember where is what will a constant
> > >> low-level pain. Things that used to be simple & quick, like git-grep
> > >> qapi/*.c, become more involved.
> > >>
> > >
> > > It's inevitable when a project grows. QEMU is already a very large
> > project. Over the years, we have structured the project, by moving things
> > and splitting in subdirectories. Imho, this is actually helpful in many
> > ways, and we got used to it easily hopefully.
> >
> > I agree with this. But generally we've tried to do that by
> > having the subdirectory splitting and naming match the
> > structure of the codebase. The exception, which I strongly
> > dislike, is that contrib/ is a grabbag of random stuff.
> > subprojects/ is starting to feel like it is also turning
> > into "place where random stuff ends up"...
> >
>
> Yes, most of contrib/* should probably be standalone projects. If only we
> had some kind of common library/subproject :)
>
> subproject/* is a meson *cough* convention (imposed location for
> subprojects). If we don't want to depend on external released libraries,
> that's just the way it is.
>
>
> >
> > > Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd,
> > storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be
> > tight to qemu code, release and management process? I am not saying it's
> > time to move them out of qemu project, but I believe it's helpful to have
> > code that is structured and can be compiled indepently.
> > >
> > > And by having "standalone" subprojects, we can more easily evolve in new
> > directions, without touching the rest of the projects.
> >
> > As Markus says, your branch ends up moving most of qobject into
> > qemu-common/. We are never going to let that out of QEMU proper,
> > because we are never going to allow ourselves to be tied to API
> > compatibility with it as an external library. So anything that
> >
>
> Why is that? We do it with a lot of dependencies already, with stable APIs.
>
> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> continue to link statically and compile against a specific version. like
> with libvfio-user today.
>
> And at this point, I am _not_ proposing to have an extra "qemu-common"
> repository. I don't think there are enough reasons to want that either.
>
>
>
> > needs qobject is never going to leave the QEMU codebase. Which
> >
> means that there's not much gain from shoving it into subproject/
> > IMHO.
>
>
> (just to be extra clear, it's qobject not QOM we are talking about)
>
> qobject is fundamental to all the QAPI related generated code. Why should
> that remain tight to QEMU proper?
Neither qobject nor QOM have ever been designed to be public APIs.
Though admittedly qobject is quite a bit simpler as an API, I'm
not convinced its current design is something we want to consider
public. As an example, just last month Markus proposed changing
QDict's implementation
https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
If we want external projects to be able to take advantage of QAPI,
the bare minimum we need to be public is a QAPI parser, from which
people can then build any code generators desired.
We don't neccessarily need the current QAPI C code generator. There
could be a new C generator that didn't use qobject, but instead used
some standard GLib types like GHashTable/GList instead of QDict/QList.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 12:15 ` Daniel P. Berrangé
@ 2022-08-11 13:35 ` Markus Armbruster
2022-08-22 8:16 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-08-11 13:35 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marc-André Lureau, Peter Maydell, Markus Armbruster,
qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Laurent Vivier, Fam Zheng, Hanna Reitz
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <peter.maydell@linaro.org>
>> wrote:
[...]
>> > As Markus says, your branch ends up moving most of qobject into
>> > qemu-common/. We are never going to let that out of QEMU proper,
>> > because we are never going to allow ourselves to be tied to API
>> > compatibility with it as an external library. So anything that
>> >
>>
>> Why is that? We do it with a lot of dependencies already, with stable APIs.
>>
>> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
>> continue to link statically and compile against a specific version. like
>> with libvfio-user today.
>>
>> And at this point, I am _not_ proposing to have an extra "qemu-common"
>> repository. I don't think there are enough reasons to want that either.
>>
>>
>>
>> > needs qobject is never going to leave the QEMU codebase. Which
>> > means that there's not much gain from shoving it into subproject/
>> > IMHO.
>>
>>
>> (just to be extra clear, it's qobject not QOM we are talking about)
>>
>> qobject is fundamental to all the QAPI related generated code. Why should
>> that remain tight to QEMU proper?
>
> Neither qobject nor QOM have ever been designed to be public APIs.
> Though admittedly qobject is quite a bit simpler as an API, I'm
> not convinced its current design is something we want to consider
> public. As an example, just last month Markus proposed changing
> QDict's implementation
>
> https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
>
>
> If we want external projects to be able to take advantage of QAPI,
> the bare minimum we need to be public is a QAPI parser, from which
> people can then build any code generators desired.
Basically scripts/qapi/ less the code generators.
Not sure a subproject would be a good fit.
Shot from the hip: could the build process spit out something external
projects could consume? It's how "consumables" are commonly delivered.
E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
it gets copied into the consuming tree ("vendored").
> We don't neccessarily need the current QAPI C code generator. There
> could be a new C generator that didn't use qobject, but instead used
> some standard GLib types like GHashTable/GList instead of QDict/QList.
Yes, that should be possible.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 13:35 ` Markus Armbruster
@ 2022-08-22 8:16 ` Marc-André Lureau
2022-09-02 11:15 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-22 8:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé, Peter Maydell, qemu-devel, Eric Blake,
Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans,
John Snow, Michael Roth, Warner Losh, Kevin Wolf,
Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Laurent Vivier, Fam Zheng, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]
Hi
On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster <armbru@redhat.com> wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <peter.maydell@linaro.org
> >
> >> wrote:
>
> [...]
>
> >> > As Markus says, your branch ends up moving most of qobject into
> >> > qemu-common/. We are never going to let that out of QEMU proper,
> >> > because we are never going to allow ourselves to be tied to API
> >> > compatibility with it as an external library. So anything that
> >> >
> >>
> >> Why is that? We do it with a lot of dependencies already, with stable
> APIs.
> >>
> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> >> continue to link statically and compile against a specific version. like
> >> with libvfio-user today.
> >>
> >> And at this point, I am _not_ proposing to have an extra "qemu-common"
> >> repository. I don't think there are enough reasons to want that either.
> >>
> >>
> >>
> >> > needs qobject is never going to leave the QEMU codebase. Which
> >> > means that there's not much gain from shoving it into subproject/
> >> > IMHO.
> >>
> >>
> >> (just to be extra clear, it's qobject not QOM we are talking about)
> >>
> >> qobject is fundamental to all the QAPI related generated code. Why
> should
> >> that remain tight to QEMU proper?
> >
> > Neither qobject nor QOM have ever been designed to be public APIs.
> > Though admittedly qobject is quite a bit simpler as an API, I'm
> > not convinced its current design is something we want to consider
> > public. As an example, just last month Markus proposed changing
> > QDict's implementation
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> >
> >
> > If we want external projects to be able to take advantage of QAPI,
> > the bare minimum we need to be public is a QAPI parser, from which
> > people can then build any code generators desired.
>
> Basically scripts/qapi/ less the code generators.
>
>
The generated code is used by qemu-ga & storage daemon, at least. They are
the first potential consumers, after qemu.
> Not sure a subproject would be a good fit.
>
(I won't repeat the arguments of structuring a project)
>
> Shot from the hip: could the build process spit out something external
> projects could consume? It's how "consumables" are commonly delivered.
> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
> it gets copied into the consuming tree ("vendored").
>
>
Not sure I follow, but that's just the "install" step isn't it ?
Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But then,
you would expect stability/versioning. That's not what I am proposing (at
least at this point), which is just about the build system and project
structure, so we can build and work on subprojects independently. (for ex,
in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
configure etc)
> > We don't neccessarily need the current QAPI C code generator. There
> > could be a new C generator that didn't use qobject, but instead used
> > some standard GLib types like GHashTable/GList instead of QDict/QList.
>
> Yes, that should be possible.
>
>
I can't see much benefit from doing that extra work. It would create two C
APIs, making future bindings efforts more difficult etc.
QObject is very much like GValue though (the naming is better too :), just
like the QOM & GObject story.
thanks
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 5398 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-22 8:16 ` Marc-André Lureau
@ 2022-09-02 11:15 ` Markus Armbruster
2022-09-02 13:22 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Markus Armbruster @ 2022-09-02 11:15 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Markus Armbruster, Daniel P. Berrangé, Peter Maydell,
qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini,
Xie Yongji, Kyle Evans, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Laurent Vivier, Fam Zheng, Hanna Reitz
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
>> >> Hi
>> >>
>> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <peter.maydell@linaro.org
>> >
>> >> wrote:
>>
>> [...]
>>
>> >> > As Markus says, your branch ends up moving most of qobject into
>> >> > qemu-common/. We are never going to let that out of QEMU proper,
>> >> > because we are never going to allow ourselves to be tied to API
>> >> > compatibility with it as an external library. So anything that
>> >> >
>> >>
>> >> Why is that? We do it with a lot of dependencies already, with stable
>> APIs.
>> >>
>> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
>> >> continue to link statically and compile against a specific version. like
>> >> with libvfio-user today.
>> >>
>> >> And at this point, I am _not_ proposing to have an extra "qemu-common"
>> >> repository. I don't think there are enough reasons to want that either.
>> >>
>> >>
>> >>
>> >> > needs qobject is never going to leave the QEMU codebase. Which
>> >> > means that there's not much gain from shoving it into subproject/
>> >> > IMHO.
>> >>
>> >>
>> >> (just to be extra clear, it's qobject not QOM we are talking about)
>> >>
>> >> qobject is fundamental to all the QAPI related generated code. Why
>> should
>> >> that remain tight to QEMU proper?
>> >
>> > Neither qobject nor QOM have ever been designed to be public APIs.
>> > Though admittedly qobject is quite a bit simpler as an API, I'm
>> > not convinced its current design is something we want to consider
>> > public. As an example, just last month Markus proposed changing
>> > QDict's implementation
>> >
>> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
>> >
>> >
>> > If we want external projects to be able to take advantage of QAPI,
>> > the bare minimum we need to be public is a QAPI parser, from which
>> > people can then build any code generators desired.
>>
>> Basically scripts/qapi/ less the code generators.
>>
>>
> The generated code is used by qemu-ga & storage daemon, at least. They are
> the first potential consumers, after qemu.
>
>
>> Not sure a subproject would be a good fit.
>>
>
> (I won't repeat the arguments of structuring a project)
>
>
>>
>> Shot from the hip: could the build process spit out something external
>> projects could consume? It's how "consumables" are commonly delivered.
>> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
>> it gets copied into the consuming tree ("vendored").
>>
>>
> Not sure I follow, but that's just the "install" step isn't it ?
>
> Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But then,
> you would expect stability/versioning. That's not what I am proposing (at
> least at this point), which is just about the build system and project
> structure, so we can build and work on subprojects independently. (for ex,
> in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
> configure etc)
I'm afraid I'm still wobbly on the benefits of subprojects, or even how
to work with them.
How exactly would we "build and work independently" on the subprojects
involving QAPI? git-clone all of QEMU, but build only a subproject (and
its dependencies)? Am I confused?
>> > We don't neccessarily need the current QAPI C code generator. There
>> > could be a new C generator that didn't use qobject, but instead used
>> > some standard GLib types like GHashTable/GList instead of QDict/QList.
>>
>> Yes, that should be possible.
>>
>>
> I can't see much benefit from doing that extra work. It would create two C
> APIs, making future bindings efforts more difficult etc.
We need to distinguish client-side and server-side APIs / bindings.
The existing C generator targets internal, server-side use. It
generates everything defined in the schema.
The existing introspection generator generates data for external,
client-side use (via QMP). It generates just the subset visible in QMP.
The proposed Go generator also targets external, client-side use (client
bindings for QMP), and should also generate just the subset visible in
QMP.
So should a future C generator of client bindings for QMP.
And then we'd have two distinct C APIs: a server-side one to help us
implement QMP, and a client-side one to help use use QMP (QMP bindings).
Whether the two would profit from sharing QObject is not clear to me
at this point.
> QObject is very much like GValue though (the naming is better too :), just
> like the QOM & GObject story.
>
> thanks
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-09-02 11:15 ` Markus Armbruster
@ 2022-09-02 13:22 ` Marc-André Lureau
0 siblings, 0 replies; 44+ messages in thread
From: Marc-André Lureau @ 2022-09-02 13:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé, Peter Maydell, qemu-devel, Eric Blake,
Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans,
John Snow, Michael Roth, Warner Losh, Kevin Wolf,
Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Laurent Vivier, Fam Zheng, Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 5975 bytes --]
Hi
On Fri, Sep 2, 2022 at 3:16 PM Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <
> peter.maydell@linaro.org
> >> >
> >> >> wrote:
> >>
> >> [...]
> >>
> >> >> > As Markus says, your branch ends up moving most of qobject into
> >> >> > qemu-common/. We are never going to let that out of QEMU proper,
> >> >> > because we are never going to allow ourselves to be tied to API
> >> >> > compatibility with it as an external library. So anything that
> >> >> >
> >> >>
> >> >> Why is that? We do it with a lot of dependencies already, with stable
> >> APIs.
> >> >>
> >> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> >> >> continue to link statically and compile against a specific version.
> like
> >> >> with libvfio-user today.
> >> >>
> >> >> And at this point, I am _not_ proposing to have an extra
> "qemu-common"
> >> >> repository. I don't think there are enough reasons to want that
> either.
> >> >>
> >> >>
> >> >>
> >> >> > needs qobject is never going to leave the QEMU codebase. Which
> >> >> > means that there's not much gain from shoving it into subproject/
> >> >> > IMHO.
> >> >>
> >> >>
> >> >> (just to be extra clear, it's qobject not QOM we are talking about)
> >> >>
> >> >> qobject is fundamental to all the QAPI related generated code. Why
> >> should
> >> >> that remain tight to QEMU proper?
> >> >
> >> > Neither qobject nor QOM have ever been designed to be public APIs.
> >> > Though admittedly qobject is quite a bit simpler as an API, I'm
> >> > not convinced its current design is something we want to consider
> >> > public. As an example, just last month Markus proposed changing
> >> > QDict's implementation
> >> >
> >> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> >> >
> >> >
> >> > If we want external projects to be able to take advantage of QAPI,
> >> > the bare minimum we need to be public is a QAPI parser, from which
> >> > people can then build any code generators desired.
> >>
> >> Basically scripts/qapi/ less the code generators.
> >>
> >>
> > The generated code is used by qemu-ga & storage daemon, at least. They
> are
> > the first potential consumers, after qemu.
> >
> >
> >> Not sure a subproject would be a good fit.
> >>
> >
> > (I won't repeat the arguments of structuring a project)
> >
> >
> >>
> >> Shot from the hip: could the build process spit out something external
> >> projects could consume? It's how "consumables" are commonly delivered.
> >> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
> >> it gets copied into the consuming tree ("vendored").
> >>
> >>
> > Not sure I follow, but that's just the "install" step isn't it ?
> >
> > Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But
> then,
> > you would expect stability/versioning. That's not what I am proposing (at
> > least at this point), which is just about the build system and project
> > structure, so we can build and work on subprojects independently. (for
> ex,
> > in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
> > configure etc)
>
> I'm afraid I'm still wobbly on the benefits of subprojects, or even how
> to work with them.
>
> How exactly would we "build and work independently" on the subprojects
> involving QAPI? git-clone all of QEMU, but build only a subproject (and
> its dependencies)? Am I confused?
>
Yes, QEMU repository would hold some subprojects (like libvhost-user
today), that you can build/develop independently with just meson / ninja.
>
> >> > We don't neccessarily need the current QAPI C code generator. There
> >> > could be a new C generator that didn't use qobject, but instead used
> >> > some standard GLib types like GHashTable/GList instead of QDict/QList.
> >>
> >> Yes, that should be possible.
> >>
> >>
> > I can't see much benefit from doing that extra work. It would create two
> C
> > APIs, making future bindings efforts more difficult etc.
>
> We need to distinguish client-side and server-side APIs / bindings.
>
Indeed.. although imho, it's best when both are similar, or use similar
types / concepts. (gdbus does a pretty good job, for ex)
>
> The existing C generator targets internal, server-side use. It
> generates everything defined in the schema.
>
> The existing introspection generator generates data for external,
> client-side use (via QMP). It generates just the subset visible in QMP.
>
> The proposed Go generator also targets external, client-side use (client
> bindings for QMP), and should also generate just the subset visible in
> QMP.
>
> So should a future C generator of client bindings for QMP.
>
> And then we'd have two distinct C APIs: a server-side one to help us
> implement QMP, and a client-side one to help use use QMP (QMP bindings).
>
In the qga subproject split wip I did, I am interested in the server side,
at this point.
>
> Whether the two would profit from sharing QObject is not clear to me
> at this point.
>
I think it would be great to provide a QObject based C client side binding,
if only for internal testing at this point. Whether to make this public and
guarantee some stability is another question.
But for now, my goal is only internal reorganization, not any
public/external API / project etc.
thanks
> > QObject is very much like GValue though (the naming is better too :),
> just
> > like the QOM & GObject story.
> >
> > thanks
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 8592 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
2022-08-11 10:22 ` Peter Maydell
2022-08-11 10:50 ` Marc-André Lureau
@ 2022-08-11 11:46 ` Markus Armbruster
1 sibling, 0 replies; 44+ messages in thread
From: Markus Armbruster @ 2022-08-11 11:46 UTC (permalink / raw)
To: Peter Maydell
Cc: Marc-André Lureau, qemu-devel, Eric Blake, Cleber Rosa,
qemu-block, Paolo Bonzini, Xie Yongji, Kyle Evans, John Snow,
Michael Roth, Warner Losh, Kevin Wolf, Dr. David Alan Gilbert,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
Peter Maydell <peter.maydell@linaro.org> writes:
> On Thu, 11 Aug 2022 at 11:09, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>> On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>>> Your moves tear closely related code apart. This is going to be a drag
>>> for developers in general and maintainers in particular.
>>>
>>> Ergonomics suffer when related code is in multiple places. Having to
>>> switch between directories and remember where is what will a constant
>>> low-level pain. Things that used to be simple & quick, like git-grep
>>> qapi/*.c, become more involved.
>>>
>>
>> It's inevitable when a project grows. QEMU is already a very large project. Over the years, we have structured the project, by moving things and splitting in subdirectories. Imho, this is actually helpful in many ways, and we got used to it easily hopefully.
>
> I agree with this. But generally we've tried to do that by
> having the subdirectory splitting and naming match the
> structure of the codebase.
Good: move a bunch of closely related files from . (tree root) and
include/ to new ./whatever/ and include/whatever/. The improvement in
organization outweighs the pain of moving.
Not so good: split some files off ./whatever into ./else/where/, even
though they are closely related.
[...]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 12/15] qemu-common: move glib-compat.h
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (10 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 11/15] qemu-common: move scripts/qapi marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 15:00 ` Warner Losh
2022-07-12 9:35 ` [PATCH v2 13/15] qemu-common: move error-report marcandre.lureau
` (2 subsequent siblings)
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
qemu-common will have compatible dependency requirements with QEMU.
Since qemu-common won't have a toplevel qemu/osdep.h which would include
various system headers, include stdbool.h (bool is used for some
declarations here).
Replace getenv() with g_getenv() to avoid extra header inclusion.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
{include => subprojects/qemu-common/include}/glib-compat.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
rename {include => subprojects/qemu-common/include}/glib-compat.h (97%)
diff --git a/include/glib-compat.h b/subprojects/qemu-common/include/glib-compat.h
similarity index 97%
rename from include/glib-compat.h
rename to subprojects/qemu-common/include/glib-compat.h
index 43a562974d80..2b0f2962f322 100644
--- a/include/glib-compat.h
+++ b/subprojects/qemu-common/include/glib-compat.h
@@ -30,6 +30,8 @@
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#include <glib.h>
+#include <stdbool.h>
+
#if defined(G_OS_UNIX)
#include <glib-unix.h>
#include <sys/types.h>
@@ -133,7 +135,7 @@ qemu_g_test_slow(void)
{
static int cached = -1;
if (cached == -1) {
- cached = g_test_slow() || getenv("G_TEST_SLOW") != NULL;
+ cached = g_test_slow() || g_getenv("G_TEST_SLOW") != NULL;
}
return cached;
}
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 12/15] qemu-common: move glib-compat.h
2022-07-12 9:35 ` [PATCH v2 12/15] qemu-common: move glib-compat.h marcandre.lureau
@ 2022-07-12 15:00 ` Warner Losh
0 siblings, 0 replies; 44+ messages in thread
From: Warner Losh @ 2022-07-12 15:00 UTC (permalink / raw)
To: Marc-André Lureau
Cc: QEMU Developers, Eric Blake, Cleber Rosa,
open list:Block layer core, Paolo Bonzini, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]
On Tue, Jul 12, 2022 at 3:37 AM <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qemu-common will have compatible dependency requirements with QEMU.
>
> Since qemu-common won't have a toplevel qemu/osdep.h which would include
> various system headers, include stdbool.h (bool is used for some
> declarations here).
>
> Replace getenv() with g_getenv() to avoid extra header inclusion.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> {include => subprojects/qemu-common/include}/glib-compat.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> rename {include => subprojects/qemu-common/include}/glib-compat.h (97%)
>
Reviewed-by: Warner Losh <imp@bsdimp.com>
>
> diff --git a/include/glib-compat.h
> b/subprojects/qemu-common/include/glib-compat.h
> similarity index 97%
> rename from include/glib-compat.h
> rename to subprojects/qemu-common/include/glib-compat.h
> index 43a562974d80..2b0f2962f322 100644
> --- a/include/glib-compat.h
> +++ b/subprojects/qemu-common/include/glib-compat.h
> @@ -30,6 +30,8 @@
> #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>
> #include <glib.h>
> +#include <stdbool.h>
> +
> #if defined(G_OS_UNIX)
> #include <glib-unix.h>
> #include <sys/types.h>
> @@ -133,7 +135,7 @@ qemu_g_test_slow(void)
> {
> static int cached = -1;
> if (cached == -1) {
> - cached = g_test_slow() || getenv("G_TEST_SLOW") != NULL;
> + cached = g_test_slow() || g_getenv("G_TEST_SLOW") != NULL;
> }
> return cached;
> }
> --
> 2.37.0.rc0
>
>
[-- Attachment #2: Type: text/html, Size: 2494 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 13/15] qemu-common: move error-report
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (11 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 12/15] qemu-common: move glib-compat.h marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT" marcandre.lureau
2022-07-12 9:35 ` [PATCH v2 15/15] qemu-common: add error-report test marcandre.lureau
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
.../qemu-common/include}/qemu/error-report.h | 2 ++
.../qemu-common/src}/error-is-detailed.c | 1 -
.../qemu-common/src}/error-report.c | 4 +++-
.../qemu-common/src/error-vprintf.c | 5 ++---
stubs/meson.build | 2 --
subprojects/qemu-common/meson.build | 20 ++++++++++++++++---
subprojects/qemu-common/src/meson.build | 5 +++++
util/meson.build | 2 +-
8 files changed, 30 insertions(+), 11 deletions(-)
rename {include => subprojects/qemu-common/include}/qemu/error-report.h (98%)
rename {stubs => subprojects/qemu-common/src}/error-is-detailed.c (77%)
rename {util => subprojects/qemu-common/src}/error-report.c (99%)
rename stubs/error-printf.c => subprojects/qemu-common/src/error-vprintf.c (78%)
create mode 100644 subprojects/qemu-common/src/meson.build
diff --git a/include/qemu/error-report.h b/subprojects/qemu-common/include/qemu/error-report.h
similarity index 98%
rename from include/qemu/error-report.h
rename to subprojects/qemu-common/include/qemu/error-report.h
index 6ab25d458350..c62dd1a633b8 100644
--- a/include/qemu/error-report.h
+++ b/subprojects/qemu-common/include/qemu/error-report.h
@@ -13,6 +13,8 @@
#ifndef QEMU_ERROR_REPORT_H
#define QEMU_ERROR_REPORT_H
+#include "glib-compat.h"
+
typedef struct Location {
/* all members are private to qemu-error.c */
enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
diff --git a/stubs/error-is-detailed.c b/subprojects/qemu-common/src/error-is-detailed.c
similarity index 77%
rename from stubs/error-is-detailed.c
rename to subprojects/qemu-common/src/error-is-detailed.c
index c47cd236932f..c3d9c3454d84 100644
--- a/stubs/error-is-detailed.c
+++ b/subprojects/qemu-common/src/error-is-detailed.c
@@ -1,4 +1,3 @@
-#include "qemu/osdep.h"
#include "qemu/error-report.h"
bool error_is_detailed(void)
diff --git a/util/error-report.c b/subprojects/qemu-common/src/error-report.c
similarity index 99%
rename from util/error-report.c
rename to subprojects/qemu-common/src/error-report.c
index 4d1d66fc0650..616428fe9579 100644
--- a/util/error-report.c
+++ b/subprojects/qemu-common/src/error-report.c
@@ -10,7 +10,9 @@
* See the COPYING file in the top-level directory.
*/
-#include "qemu/osdep.h"
+#include <stdio.h>
+#include <assert.h>
+
#include "qemu/error-report.h"
/*
diff --git a/stubs/error-printf.c b/subprojects/qemu-common/src/error-vprintf.c
similarity index 78%
rename from stubs/error-printf.c
rename to subprojects/qemu-common/src/error-vprintf.c
index 1afa0f62ca26..b815d88dfe65 100644
--- a/stubs/error-printf.c
+++ b/subprojects/qemu-common/src/error-vprintf.c
@@ -1,13 +1,12 @@
-#include "qemu/osdep.h"
+#include <stdio.h>
#include "qemu/error-report.h"
-#include "monitor/monitor.h"
int error_vprintf(const char *fmt, va_list ap)
{
int ret;
if (g_test_initialized() && !g_test_subprocess() &&
- getenv("QTEST_SILENT_ERRORS")) {
+ g_getenv("QTEST_SILENT_ERRORS")) {
char *msg = g_strdup_vprintf(fmt, ap);
g_test_message("%s", msg);
ret = strlen(msg);
diff --git a/stubs/meson.build b/stubs/meson.build
index 0f3a782824f9..498b6ee0466e 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -9,8 +9,6 @@ stub_ss.add(files('cpus-get-virtual-clock.c'))
stub_ss.add(files('qemu-timer-notify-cb.c'))
stub_ss.add(files('icount.c'))
stub_ss.add(files('dump.c'))
-stub_ss.add(files('error-is-detailed.c'))
-stub_ss.add(files('error-printf.c'))
stub_ss.add(files('fdset.c'))
stub_ss.add(files('gdbstub.c'))
stub_ss.add(files('get-vm-name.c'))
diff --git a/subprojects/qemu-common/meson.build b/subprojects/qemu-common/meson.build
index 207e53991061..05bca6d30d49 100644
--- a/subprojects/qemu-common/meson.build
+++ b/subprojects/qemu-common/meson.build
@@ -3,8 +3,22 @@ project('qemu-common', 'c',
default_options: ['c_std=gnu11']
)
-qemu_common_dep = declare_dependency(
- include_directories: include_directories('include'),
-)
+glib_dep = dependency('glib-2.0')
+inc = include_directories('include')
+
+sources = []
subdir('scripts')
+subdir('src')
+
+lib = static_library(
+ 'qemu-common', sources,
+ dependencies: [glib_dep],
+ include_directories: inc,
+)
+
+qemu_common_dep = declare_dependency(
+ link_with: lib,
+ include_directories: inc,
+ dependencies: [glib_dep],
+)
diff --git a/subprojects/qemu-common/src/meson.build b/subprojects/qemu-common/src/meson.build
new file mode 100644
index 000000000000..d85a314065e5
--- /dev/null
+++ b/subprojects/qemu-common/src/meson.build
@@ -0,0 +1,5 @@
+sources += files(
+ 'error-is-detailed.c',
+ 'error-report.c',
+ 'error-vprintf.c',
+)
diff --git a/util/meson.build b/util/meson.build
index 8cce8f8968f6..4dae6f815e31 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -28,7 +28,7 @@ util_ss.add(files('host-utils.c'))
util_ss.add(files('bitmap.c', 'bitops.c'))
util_ss.add(files('fifo8.c'))
util_ss.add(files('cacheflush.c'))
-util_ss.add(files('error.c', 'error-report.c'))
+util_ss.add(files('error.c'))
util_ss.add(files('qemu-print.c'))
util_ss.add(files('id.c'))
util_ss.add(files('qemu-config.c', 'notify.c'))
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT"
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (12 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 13/15] qemu-common: move error-report marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
2022-08-05 10:35 ` Paolo Bonzini
2022-07-12 9:35 ` [PATCH v2 15/15] qemu-common: add error-report test marcandre.lureau
14 siblings, 1 reply; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
A subproject test may be simply in the "PROJECT" suite (such as
"qemu-common" with the following patches)
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/mtest2make.py | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 0fe81efbbcec..606821ee2732 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -51,8 +51,11 @@ def process_tests(test, targets, suites):
test_suites = test['suite'] or ['default']
for s in test_suites:
- # The suite name in the introspection info is "PROJECT:SUITE"
- s = s.split(':')[1]
+ # The suite name in the introspection info is "PROJECT" or "PROJECT:SUITE"
+ try:
+ s = s.split(':')[1]
+ except IndexError:
+ continue
if s == 'slow' or s == 'thorough':
continue
if s.endswith('-slow'):
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT"
2022-07-12 9:35 ` [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT" marcandre.lureau
@ 2022-08-05 10:35 ` Paolo Bonzini
2022-08-05 11:22 ` Marc-André Lureau
0 siblings, 1 reply; 44+ messages in thread
From: Paolo Bonzini @ 2022-08-05 10:35 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Xie Yongji, Kyle Evans,
Peter Maydell, John Snow, Michael Roth, Warner Losh, Kevin Wolf,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
On 7/12/22 11:35, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> A subproject test may be simply in the "PROJECT" suite (such as
> "qemu-common" with the following patches)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/mtest2make.py | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
> index 0fe81efbbcec..606821ee2732 100644
> --- a/scripts/mtest2make.py
> +++ b/scripts/mtest2make.py
> @@ -51,8 +51,11 @@ def process_tests(test, targets, suites):
>
> test_suites = test['suite'] or ['default']
> for s in test_suites:
> - # The suite name in the introspection info is "PROJECT:SUITE"
> - s = s.split(':')[1]
> + # The suite name in the introspection info is "PROJECT" or "PROJECT:SUITE"
> + try:
> + s = s.split(':')[1]
> + except IndexError:
> + continue
Shouldn't you continue with s begin simply "PROJECT"? That is, just
if ':' in s:
s = s.split(':')[1]
This way you can do "make check-qemu-common".
> if s == 'slow' or s == 'thorough':
> continue
> if s.endswith('-slow'):
and then however these two "ifs" need to be under the case where the
suite name is "PROJECT:SUITE".
Paolo
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT"
2022-08-05 10:35 ` Paolo Bonzini
@ 2022-08-05 11:22 ` Marc-André Lureau
0 siblings, 0 replies; 44+ messages in thread
From: Marc-André Lureau @ 2022-08-05 11:22 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Eric Blake, Cleber Rosa, qemu-block, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz
[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]
Hi
On Fri, Aug 5, 2022 at 2:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 7/12/22 11:35, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > A subproject test may be simply in the "PROJECT" suite (such as
> > "qemu-common" with the following patches)
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > scripts/mtest2make.py | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
> > index 0fe81efbbcec..606821ee2732 100644
> > --- a/scripts/mtest2make.py
> > +++ b/scripts/mtest2make.py
> > @@ -51,8 +51,11 @@ def process_tests(test, targets, suites):
> >
> > test_suites = test['suite'] or ['default']
> > for s in test_suites:
> > - # The suite name in the introspection info is "PROJECT:SUITE"
> > - s = s.split(':')[1]
> > + # The suite name in the introspection info is "PROJECT" or
> "PROJECT:SUITE"
> > + try:
> > + s = s.split(':')[1]
> > + except IndexError:
> > + continue
>
> Shouldn't you continue with s begin simply "PROJECT"? That is, just
>
> if ':' in s:
> s = s.split(':')[1]
>
> This way you can do "make check-qemu-common".
>
> > if s == 'slow' or s == 'thorough':
> > continue
> > if s.endswith('-slow'):
>
> and then however these two "ifs" need to be under the case where the
> suite name is "PROJECT:SUITE".
>
>
Thanks for the tips, updated
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2646 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 15/15] qemu-common: add error-report test
2022-07-12 9:35 [PATCH v2 00/15] Preliminary patches for subproject split marcandre.lureau
` (13 preceding siblings ...)
2022-07-12 9:35 ` [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT" marcandre.lureau
@ 2022-07-12 9:35 ` marcandre.lureau
14 siblings, 0 replies; 44+ messages in thread
From: marcandre.lureau @ 2022-07-12 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Cleber Rosa, qemu-block, Paolo Bonzini, Xie Yongji,
Kyle Evans, Peter Maydell, John Snow, Michael Roth, Warner Losh,
Kevin Wolf, Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Laurent Vivier, Fam Zheng,
Hanna Reitz, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add new tests to check the behaviour of error reporting functions.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
.../qemu-common/tests/test-error-report.c | 120 ++++++++++++++++++
subprojects/qemu-common/meson.build | 2 +
subprojects/qemu-common/tests/meson.build | 12 ++
3 files changed, 134 insertions(+)
create mode 100644 subprojects/qemu-common/tests/test-error-report.c
create mode 100644 subprojects/qemu-common/tests/meson.build
diff --git a/subprojects/qemu-common/tests/test-error-report.c b/subprojects/qemu-common/tests/test-error-report.c
new file mode 100644
index 000000000000..09a2d122a0e1
--- /dev/null
+++ b/subprojects/qemu-common/tests/test-error-report.c
@@ -0,0 +1,120 @@
+/*
+ * Error reporting test
+ *
+ * Copyright (C) 2022 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "glib-compat.h"
+#include <locale.h>
+
+#include "qemu/error-report.h"
+
+static void
+test_error_report_simple(void)
+{
+ if (g_test_subprocess()) {
+ error_report("%s", "test error");
+ warn_report("%s", "test warn");
+ info_report("%s", "test info");
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("\
+test-error-report: test error*\
+test-error-report: warning: test warn*\
+test-error-report: info: test info*\
+");
+}
+
+static void
+test_error_report_loc(void)
+{
+ if (g_test_subprocess()) {
+ loc_set_file("some-file.c", 7717);
+ error_report("%s", "test error1");
+ loc_set_none();
+ error_report("%s", "test error2");
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("\
+test-error-report:some-file.c:7717: test error1*\
+test-error-report: test error2*\
+");
+}
+
+static void
+test_error_report_glog(void)
+{
+ if (g_test_subprocess()) {
+ g_message("gmessage");
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("test-error-report: info: gmessage*");
+}
+
+static void
+test_error_report_once(void)
+{
+ int i;
+
+ if (g_test_subprocess()) {
+ for (i = 0; i < 3; i++) {
+ warn_report_once("warn");
+ error_report_once("err");
+ }
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("\
+test-error-report: warning: warn*\
+test-error-report: err*\
+");
+}
+
+static void
+test_error_report_timestamp(void)
+{
+ if (g_test_subprocess()) {
+ message_with_timestamp = true;
+ warn_report("warn");
+ error_report("err");
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("\
+*-*-*:*:* test-error-report: warning: warn*\
+*-*-*:*:* test-error-report: err*\
+");
+}
+
+int
+main(int argc, char *argv[])
+{
+ setlocale(LC_ALL, "");
+
+ g_test_init(&argc, &argv, NULL);
+ error_init("test-error-report");
+
+ g_test_add_func("/error-report/simple", test_error_report_simple);
+ g_test_add_func("/error-report/loc", test_error_report_loc);
+ g_test_add_func("/error-report/glog", test_error_report_glog);
+ g_test_add_func("/error-report/once", test_error_report_once);
+ g_test_add_func("/error-report/timestamp", test_error_report_timestamp);
+
+ return g_test_run();
+}
diff --git a/subprojects/qemu-common/meson.build b/subprojects/qemu-common/meson.build
index 05bca6d30d49..991aaac52040 100644
--- a/subprojects/qemu-common/meson.build
+++ b/subprojects/qemu-common/meson.build
@@ -22,3 +22,5 @@ qemu_common_dep = declare_dependency(
include_directories: inc,
dependencies: [glib_dep],
)
+
+subdir('tests')
diff --git a/subprojects/qemu-common/tests/meson.build b/subprojects/qemu-common/tests/meson.build
new file mode 100644
index 000000000000..3dd10c180b50
--- /dev/null
+++ b/subprojects/qemu-common/tests/meson.build
@@ -0,0 +1,12 @@
+env = [
+ 'G_TEST_SRCDIR=@0@'.format(meson.current_source_dir()),
+ 'G_TEST_BUILDDIR=@0@'.format(meson.current_build_dir()),
+]
+
+test('error-report',
+ executable('test-error-report',
+ sources: files('test-error-report.c'),
+ dependencies: qemu_common_dep,
+ ),
+ env: env,
+)
--
2.37.0.rc0
^ permalink raw reply related [flat|nested] 44+ messages in thread