[PATCH 0/9] Preliminary patches for subproject split

[PATCH 0/9] Preliminary patches for subproject split

[marcandre.lureau]

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

Hi,

Here is another subset of the large "subproject(qga)"" series I intend to send
soon after (https://gitlab.com/marcandre.lureau/qemu/-/commits/qga).

Thanks

Marc-André Lureau (9):
  monitor: make error_vprintf_unless_qmp() static
  error-report: misc comment fix
  error-report: introduce "detailed" variable
  error-report: simplify print_loc()
  error-report: introduce ErrorReportDetailedFunc
  error-report: add a callback to overwrite error_vprintf
  qapi: move QEMU-specific dispatch code in monitor
  scripts/qapi-gen: add -i option
  scripts/qapi: add required system includes to visitor

 include/monitor/monitor.h            |  2 +-
 include/qapi/qmp/dispatch.h          |  7 ++-
 include/qemu/error-report.h          |  8 +++-
 bsd-user/main.c                      |  2 +-
 linux-user/main.c                    |  2 +-
 monitor/monitor.c                    |  5 +-
 monitor/qmp.c                        | 68 +++++++++++++++++++++++++++-
 qapi/qmp-dispatch.c                  | 64 ++------------------------
 qemu-img.c                           |  2 +-
 qemu-io.c                            |  2 +-
 qemu-nbd.c                           |  2 +-
 qga/main.c                           |  2 +-
 scsi/qemu-pr-helper.c                |  2 +-
 softmmu/vl.c                         |  7 ++-
 storage-daemon/qemu-storage-daemon.c |  7 ++-
 stubs/error-printf.c                 | 23 ----------
 tests/unit/test-qmp-cmds.c           |  6 +--
 util/error-report.c                  | 46 ++++++++++++++-----
 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                | 21 ++++++---
 stubs/meson.build                    |  1 -
 26 files changed, 226 insertions(+), 147 deletions(-)
 delete mode 100644 stubs/error-printf.c

-- 
2.37.0.rc0

[PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[marcandre.lureau]

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

Not needed outside monitor.c. Remove the needless stub.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/monitor/monitor.h | 1 -
 monitor/monitor.c         | 3 ++-
 stubs/error-printf.c      | 5 -----
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a4b40e8391db..44653e195b45 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
 void monitor_register_hmp_info_hrt(const char *name,
                                    HumanReadableText *(*handler)(Error **errp));
 
-int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
 int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
 
 #endif /* MONITOR_H */
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 86949024f643..ba4c1716a48a 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -273,7 +273,8 @@ 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)
+G_GNUC_PRINTF(1, 0)
+static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
     Monitor *cur_mon = monitor_cur();
 
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

Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Not needed outside monitor.c. Remove the needless stub.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/monitor/monitor.h | 1 -
>  monitor/monitor.c         | 3 ++-
>  stubs/error-printf.c      | 5 -----
>  3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index a4b40e8391db..44653e195b45 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
>  void monitor_register_hmp_info_hrt(const char *name,
>                                     HumanReadableText *(*handler)(Error **errp));
>  
> -int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
>  int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
>  
>  #endif /* MONITOR_H */
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 86949024f643..ba4c1716a48a 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -273,7 +273,8 @@ 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)
> +G_GNUC_PRINTF(1, 0)
> +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  {
>      Monitor *cur_mon = monitor_cur();
>  
> 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);
> -}

When I write a printf-like utility function, I habitually throw in a
vprintf-like function.

Any particular reason for hiding this one?  To avoid misunderstandings:
I'm fine with hiding it if it's causing you trouble.

Except I think we'd better delete than hide then: inline into
error_printf_unless_qmp().  Makes sense?

Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[Marc-André Lureau]

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

Hi

On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Not needed outside monitor.c. Remove the needless stub.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/monitor/monitor.h | 1 -
> >  monitor/monitor.c         | 3 ++-
> >  stubs/error-printf.c      | 5 -----
> >  3 files changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index a4b40e8391db..44653e195b45 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
> >  void monitor_register_hmp_info_hrt(const char *name,
> >                                     HumanReadableText *(*handler)(Error
> **errp));
> >
> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> G_GNUC_PRINTF(1, 0);
> >  int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
> >
> >  #endif /* MONITOR_H */
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 86949024f643..ba4c1716a48a 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -273,7 +273,8 @@ 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)
> > +G_GNUC_PRINTF(1, 0)
> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >  {
> >      Monitor *cur_mon = monitor_cur();
> >
> > 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);
> > -}
>
> When I write a printf-like utility function, I habitually throw in a
> vprintf-like function.
>
> Any particular reason for hiding this one?  To avoid misunderstandings:
> I'm fine with hiding it if it's causing you trouble.
>

I don't think I had an issue with it, only that I wrote tests for the
error-report.h API, and didn't see the need to cover a function that isn't
used outside the unit.


>
> Except I think we'd better delete than hide then: inline into
> error_printf_unless_qmp().  Makes sense?
>

It can't be easily inlined because of the surrounding va_start/va_end


-- 
Marc-André Lureau

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

Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Not needed outside monitor.c. Remove the needless stub.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  include/monitor/monitor.h | 1 -
>> >  monitor/monitor.c         | 3 ++-
>> >  stubs/error-printf.c      | 5 -----
>> >  3 files changed, 2 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index a4b40e8391db..44653e195b45 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
>> >  void monitor_register_hmp_info_hrt(const char *name,
>> >                                     HumanReadableText *(*handler)(Error **errp));
>> >
>> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
>> >  int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
>> >
>> >  #endif /* MONITOR_H */
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 86949024f643..ba4c1716a48a 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -273,7 +273,8 @@ 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)
>> > +G_GNUC_PRINTF(1, 0)
>> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>> >  {
>> >      Monitor *cur_mon = monitor_cur();
>> >
>> > 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);
>> > -}
>>
>> When I write a printf-like utility function, I habitually throw in a
>> vprintf-like function.
>>
>> Any particular reason for hiding this one?  To avoid misunderstandings:
>> I'm fine with hiding it if it's causing you trouble.
>
> I don't think I had an issue with it, only that I wrote tests for the
> error-report.h API, and didn't see the need to cover a function that isn't
> used outside the unit.

I'd keep it and not worry about missing tests; the tests of
error_printf_unless_qmp() exercise it fine.

>> Except I think we'd better delete than hide then: inline into
>> error_printf_unless_qmp().  Makes sense?
>
> It can't be easily inlined because of the surrounding va_start/va_end

Easily enough, I think:

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 86949024f6..201a672ac6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -273,27 +273,22 @@ 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)
-{
-    Monitor *cur_mon = monitor_cur();
-
-    if (!cur_mon) {
-        return vfprintf(stderr, fmt, ap);
-    }
-    if (!monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
-    }
-    return -1;
-}
-
 int error_printf_unless_qmp(const char *fmt, ...)
 {
+    Monitor *cur_mon = monitor_cur();
     va_list ap;
     int ret;
 
     va_start(ap, fmt);
-    ret = error_vprintf_unless_qmp(fmt, ap);
+    if (!cur_mon) {
+        ret = vfprintf(stderr, fmt, ap);
+    } else if (!monitor_cur_is_qmp()) {
+        ret = monitor_vprintf(cur_mon, fmt, ap);
+    } else {
+        ret = -1;
+    }
     va_end(ap);
+
     return ret;
 }
 

Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[Marc-André Lureau]

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

Hi

On Fri, Jul 8, 2022 at 5:56 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > Not needed outside monitor.c. Remove the needless stub.
> >> >
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > ---
> >> >  include/monitor/monitor.h | 1 -
> >> >  monitor/monitor.c         | 3 ++-
> >> >  stubs/error-printf.c      | 5 -----
> >> >  3 files changed, 2 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> > index a4b40e8391db..44653e195b45 100644
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool
> info,
> >> >  void monitor_register_hmp_info_hrt(const char *name,
> >> >                                     HumanReadableText
> *(*handler)(Error **errp));
> >> >
> >> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> G_GNUC_PRINTF(1, 0);
> >> >  int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1,
> 2);
> >> >
> >> >  #endif /* MONITOR_H */
> >> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> > index 86949024f643..ba4c1716a48a 100644
> >> > --- a/monitor/monitor.c
> >> > +++ b/monitor/monitor.c
> >> > @@ -273,7 +273,8 @@ 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)
> >> > +G_GNUC_PRINTF(1, 0)
> >> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >> >  {
> >> >      Monitor *cur_mon = monitor_cur();
> >> >
> >> > 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);
> >> > -}
> >>
> >> When I write a printf-like utility function, I habitually throw in a
> >> vprintf-like function.
> >>
> >> Any particular reason for hiding this one?  To avoid misunderstandings:
> >> I'm fine with hiding it if it's causing you trouble.
> >
> > I don't think I had an issue with it, only that I wrote tests for the
> > error-report.h API, and didn't see the need to cover a function that
> isn't
> > used outside the unit.
>
> I'd keep it and not worry about missing tests; the tests of
> error_printf_unless_qmp() exercise it fine.
>

ok


>
> >> Except I think we'd better delete than hide then: inline into
> >> error_printf_unless_qmp().  Makes sense?
> >
> > It can't be easily inlined because of the surrounding va_start/va_end
>
> Easily enough, I think:
>

ah yes indeed! :)


>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 86949024f6..201a672ac6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -273,27 +273,22 @@ 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)
> -{
> -    Monitor *cur_mon = monitor_cur();
> -
> -    if (!cur_mon) {
> -        return vfprintf(stderr, fmt, ap);
> -    }
> -    if (!monitor_cur_is_qmp()) {
> -        return monitor_vprintf(cur_mon, fmt, ap);
> -    }
> -    return -1;
> -}
> -
>  int error_printf_unless_qmp(const char *fmt, ...)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      va_list ap;
>      int ret;
>
>      va_start(ap, fmt);
> -    ret = error_vprintf_unless_qmp(fmt, ap);
> +    if (!cur_mon) {
> +        ret = vfprintf(stderr, fmt, ap);
> +    } else if (!monitor_cur_is_qmp()) {
> +        ret = monitor_vprintf(cur_mon, fmt, ap);
> +    } else {
> +        ret = -1;
> +    }
>      va_end(ap);
> +
>      return ret;
>  }
>
>
>

-- 
Marc-André Lureau

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

[PATCH 2/9] error-report: misc comment fix

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@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

Re: [PATCH 2/9] error-report: misc comment fix

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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);
>  
>      /*

Missed in commit beeb175c0d "util/qemu-error: Rename error_print_loc()
to be more generic".

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

[PATCH 3/9] error-report: introduce "detailed" variable

[marcandre.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>
---
 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

Re: [PATCH 3/9] error-report: introduce "detailed" variable

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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>
> ---
>  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);
>      }

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

[PATCH 4/9] error-report: simplify print_loc()

[marcandre.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>
---
 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

Re: [PATCH 4/9] error-report: simplify print_loc()

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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>
> ---
>  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.

Document the argument?  Not sure it's worth the bother...

>   */
> -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:

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

[PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[marcandre.lureau]

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

Remove monitor dependency from error printing code, by allowing programs
to set a callback for when to use "detailed" reporting or not.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/error-report.h          | 4 +++-
 bsd-user/main.c                      | 2 +-
 linux-user/main.c                    | 2 +-
 qemu-img.c                           | 2 +-
 qemu-io.c                            | 2 +-
 qemu-nbd.c                           | 2 +-
 scsi/qemu-pr-helper.c                | 2 +-
 softmmu/vl.c                         | 7 ++++++-
 storage-daemon/qemu-storage-daemon.c | 7 ++++++-
 util/error-report.c                  | 8 +++++---
 10 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3ae2357fda54..e2e630f207f0 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -13,6 +13,8 @@
 #ifndef QEMU_ERROR_REPORT_H
 #define QEMU_ERROR_REPORT_H
 
+typedef bool (*ErrorReportDetailedFunc)(void);
+
 typedef struct Location {
     /* all members are private to qemu-error.c */
     enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
@@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char *fmt, ...)
 bool warn_report_once_cond(bool *printed, const char *fmt, ...)
     G_GNUC_PRINTF(2, 3);
 
-void error_init(const char *argv0);
+void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
 
 /*
  * Similar to error_report(), except it prints the message just once.
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 6f09180d6541..d5f8fca863d7 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -292,7 +292,7 @@ int main(int argc, char **argv)
 
     save_proc_pathname(argv[0]);
 
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_cpu_list();
     module_call_init(MODULE_INIT_QOM);
diff --git a/linux-user/main.c b/linux-user/main.c
index 651e32f5f248..84f380bd366d 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -646,7 +646,7 @@ int main(int argc, char **argv, char **envp)
     unsigned long max_reserved_va;
     bool preserve_argv0;
 
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_cpu_list();
     module_call_init(MODULE_INIT_QOM);
diff --git a/qemu-img.c b/qemu-img.c
index 4cf4d2423df8..1f27a9fc70f6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5396,7 +5396,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
 
diff --git a/qemu-io.c b/qemu-io.c
index 2bd7bfb65073..b5cdc7c922a7 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -539,7 +539,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0cd5aa6f02bc..6bc632c93611 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -587,7 +587,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     qcrypto_init(&error_fatal);
 
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 196b78c00df5..8d80e58d4498 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -910,7 +910,7 @@ int main(int argc, char **argv)
 
     signal(SIGPIPE, SIG_IGN);
 
-    error_init(argv[0]);
+    error_init(argv[0], NULL);
     module_call_init(MODULE_INIT_TRACE);
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_trace_opts);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 54e920ada1a1..3b46fc9c1fc5 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
     }
 }
 
+static bool error_is_detailed(void)
+{
+    return !monitor_cur();
+}
+
 void qemu_init(int argc, char **argv, char **envp)
 {
     QemuOpts *opts;
@@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_action_opts);
     module_call_init(MODULE_INIT_OPTS);
 
-    error_init(argv[0]);
+    error_init(argv[0], error_is_detailed);
     qemu_init_exec_dir(argv[0]);
 
     qemu_init_arch_modules();
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index c104817cdddc..7e4d5030a045 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -368,13 +368,18 @@ static void pid_file_init(void)
     atexit(pid_file_cleanup);
 }
 
+static bool error_is_detailed(void)
+{
+    return !monitor_cur();
+}
+
 int main(int argc, char *argv[])
 {
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
 #endif
 
-    error_init(argv[0]);
+    error_init(argv[0], error_is_detailed);
     qemu_init_exec_dir(argv[0]);
     os_setup_signal_handling();
 
diff --git a/util/error-report.c b/util/error-report.c
index c43227a975e2..c2181f80a83d 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"
 
 /*
@@ -28,6 +27,7 @@ typedef enum {
 bool message_with_timestamp;
 bool error_with_guestname;
 const char *error_guest_name;
+ErrorReportDetailedFunc detailed_fn = NULL;
 
 int error_printf(const char *fmt, ...)
 {
@@ -195,7 +195,7 @@ real_time_iso8601(void)
  */
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
-    bool detailed = !monitor_cur();
+    bool detailed = detailed_fn ? detailed_fn() : TRUE;
     gchar *timestr;
 
     if (message_with_timestamp && detailed) {
@@ -387,7 +387,7 @@ static void qemu_log_func(const gchar *log_domain,
     }
 }
 
-void error_init(const char *argv0)
+void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
 {
     const char *p = strrchr(argv0, '/');
 
@@ -401,4 +401,6 @@ void error_init(const char *argv0)
     g_log_set_default_handler(qemu_log_func, NULL);
     g_warn_if_fail(qemu_glog_domains == NULL);
     qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
+
+    detailed_fn = detailed;
 }
-- 
2.37.0.rc0

Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[Warner Losh]

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

On Thu, Jun 16, 2022 at 6:41 AM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove monitor dependency from error printing code, by allowing programs
> to set a callback for when to use "detailed" reporting or not.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/error-report.h          | 4 +++-
>  bsd-user/main.c                      | 2 +-
>  linux-user/main.c                    | 2 +-
>  qemu-img.c                           | 2 +-
>  qemu-io.c                            | 2 +-
>  qemu-nbd.c                           | 2 +-
>  scsi/qemu-pr-helper.c                | 2 +-
>  softmmu/vl.c                         | 7 ++++++-
>  storage-daemon/qemu-storage-daemon.c | 7 ++++++-
>  util/error-report.c                  | 8 +++++---
>  10 files changed, 26 insertions(+), 12 deletions(-)
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

While all the changes look good, I'm only confident about the *-user
changes if that matters.

Warner


> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..e2e630f207f0 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_ERROR_REPORT_H
>  #define QEMU_ERROR_REPORT_H
>
> +typedef bool (*ErrorReportDetailedFunc)(void);
> +
>  typedef struct Location {
>      /* all members are private to qemu-error.c */
>      enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char
> *fmt, ...)
>  bool warn_report_once_cond(bool *printed, const char *fmt, ...)
>      G_GNUC_PRINTF(2, 3);
>
> -void error_init(const char *argv0);
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
>
>  /*
>   * Similar to error_report(), except it prints the message just once.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d6541..d5f8fca863d7 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -292,7 +292,7 @@ int main(int argc, char **argv)
>
>      save_proc_pathname(argv[0]);
>
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_cpu_list();
>      module_call_init(MODULE_INIT_QOM);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 651e32f5f248..84f380bd366d 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -646,7 +646,7 @@ int main(int argc, char **argv, char **envp)
>      unsigned long max_reserved_va;
>      bool preserve_argv0;
>
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_cpu_list();
>      module_call_init(MODULE_INIT_QOM);
> diff --git a/qemu-img.c b/qemu-img.c
> index 4cf4d2423df8..1f27a9fc70f6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -5396,7 +5396,7 @@ int main(int argc, char **argv)
>  #endif
>
>      socket_init();
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_exec_dir(argv[0]);
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 2bd7bfb65073..b5cdc7c922a7 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -539,7 +539,7 @@ int main(int argc, char **argv)
>  #endif
>
>      socket_init();
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_exec_dir(argv[0]);
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0cd5aa6f02bc..6bc632c93611 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -587,7 +587,7 @@ int main(int argc, char **argv)
>  #endif
>
>      socket_init();
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qcrypto_init(&error_fatal);
>
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 196b78c00df5..8d80e58d4498 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -910,7 +910,7 @@ int main(int argc, char **argv)
>
>      signal(SIGPIPE, SIG_IGN);
>
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      module_call_init(MODULE_INIT_QOM);
>      qemu_add_opts(&qemu_trace_opts);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 54e920ada1a1..3b46fc9c1fc5 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
>      }
>  }
>
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>      QemuOpts *opts;
> @@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_action_opts);
>      module_call_init(MODULE_INIT_OPTS);
>
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>
>      qemu_init_arch_modules();
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index c104817cdddc..7e4d5030a045 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -368,13 +368,18 @@ static void pid_file_init(void)
>      atexit(pid_file_cleanup);
>  }
>
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  #ifdef CONFIG_POSIX
>      signal(SIGPIPE, SIG_IGN);
>  #endif
>
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>      os_setup_signal_handling();
>
> diff --git a/util/error-report.c b/util/error-report.c
> index c43227a975e2..c2181f80a83d 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"
>
>  /*
> @@ -28,6 +27,7 @@ typedef enum {
>  bool message_with_timestamp;
>  bool error_with_guestname;
>  const char *error_guest_name;
> +ErrorReportDetailedFunc detailed_fn = NULL;
>
>  int error_printf(const char *fmt, ...)
>  {
> @@ -195,7 +195,7 @@ real_time_iso8601(void)
>   */
>  static void vreport(report_type type, const char *fmt, va_list ap)
>  {
> -    bool detailed = !monitor_cur();
> +    bool detailed = detailed_fn ? detailed_fn() : TRUE;
>      gchar *timestr;
>
>      if (message_with_timestamp && detailed) {
> @@ -387,7 +387,7 @@ static void qemu_log_func(const gchar *log_domain,
>      }
>  }
>
> -void error_init(const char *argv0)
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
>  {
>      const char *p = strrchr(argv0, '/');
>
> @@ -401,4 +401,6 @@ void error_init(const char *argv0)
>      g_log_set_default_handler(qemu_log_func, NULL);
>      g_warn_if_fail(qemu_glog_domains == NULL);
>      qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
> +
> +    detailed_fn = detailed;
>  }
> --
> 2.37.0.rc0
>
>

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

Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove monitor dependency from error printing code, by allowing programs
> to set a callback for when to use "detailed" reporting or not.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/error-report.h          | 4 +++-
>  bsd-user/main.c                      | 2 +-
>  linux-user/main.c                    | 2 +-
>  qemu-img.c                           | 2 +-
>  qemu-io.c                            | 2 +-
>  qemu-nbd.c                           | 2 +-
>  scsi/qemu-pr-helper.c                | 2 +-
>  softmmu/vl.c                         | 7 ++++++-
>  storage-daemon/qemu-storage-daemon.c | 7 ++++++-
>  util/error-report.c                  | 8 +++++---
>  10 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..e2e630f207f0 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_ERROR_REPORT_H
>  #define QEMU_ERROR_REPORT_H
>  
> +typedef bool (*ErrorReportDetailedFunc)(void);
> +
>  typedef struct Location {
>      /* all members are private to qemu-error.c */
>      enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char *fmt, ...)
>  bool warn_report_once_cond(bool *printed, const char *fmt, ...)
>      G_GNUC_PRINTF(2, 3);
>  
> -void error_init(const char *argv0);
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
>  
>  /*
>   * Similar to error_report(), except it prints the message just once.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d6541..d5f8fca863d7 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -292,7 +292,7 @@ int main(int argc, char **argv)
>  
>      save_proc_pathname(argv[0]);
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_cpu_list();
>      module_call_init(MODULE_INIT_QOM);
[More such calls of error_init()...]
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 54e920ada1a1..3b46fc9c1fc5 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
>      }
>  }
>  
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>      QemuOpts *opts;
> @@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_action_opts);
>      module_call_init(MODULE_INIT_OPTS);
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>  
>      qemu_init_arch_modules();
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index c104817cdddc..7e4d5030a045 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -368,13 +368,18 @@ static void pid_file_init(void)
>      atexit(pid_file_cleanup);
>  }
>  
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  #ifdef CONFIG_POSIX
>      signal(SIGPIPE, SIG_IGN);
>  #endif
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>      os_setup_signal_handling();
>  
> diff --git a/util/error-report.c b/util/error-report.c
> index c43227a975e2..c2181f80a83d 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"
>  
>  /*
> @@ -28,6 +27,7 @@ typedef enum {
>  bool message_with_timestamp;
>  bool error_with_guestname;
>  const char *error_guest_name;
> +ErrorReportDetailedFunc detailed_fn = NULL;
>  
>  int error_printf(const char *fmt, ...)
>  {
> @@ -195,7 +195,7 @@ real_time_iso8601(void)
>   */
>  static void vreport(report_type type, const char *fmt, va_list ap)
>  {
> -    bool detailed = !monitor_cur();
> +    bool detailed = detailed_fn ? detailed_fn() : TRUE;
>      gchar *timestr;
>  
>      if (message_with_timestamp && detailed) {
> @@ -387,7 +387,7 @@ static void qemu_log_func(const gchar *log_domain,
>      }
>  }
>  
> -void error_init(const char *argv0)
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
>  {
>      const char *p = strrchr(argv0, '/');
>  
> @@ -401,4 +401,6 @@ void error_init(const char *argv0)
>      g_log_set_default_handler(qemu_log_func, NULL);
>      g_warn_if_fail(qemu_glog_domains == NULL);
>      qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
> +
> +    detailed_fn = detailed;
>  }

A callback works, but note that each program's function is fixed.  Why
not use the linker to resolve it?  Have a .o in libqemuutil.a that
defines error_is_detailed() to return false always.  Have another
error_is_detailed() that returns !monitor_cur() in monitor.c.  A program
that links the monitor gets the latter, all the others the former.

What do you think?

Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[Marc-André Lureau]

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

Hi

On Thu, Jul 7, 2022 at 4:13 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Remove monitor dependency from error printing code, by allowing programs
> > to set a callback for when to use "detailed" reporting or not.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qemu/error-report.h          | 4 +++-
> >  bsd-user/main.c                      | 2 +-
> >  linux-user/main.c                    | 2 +-
> >  qemu-img.c                           | 2 +-
> >  qemu-io.c                            | 2 +-
> >  qemu-nbd.c                           | 2 +-
> >  scsi/qemu-pr-helper.c                | 2 +-
> >  softmmu/vl.c                         | 7 ++++++-
> >  storage-daemon/qemu-storage-daemon.c | 7 ++++++-
> >  util/error-report.c                  | 8 +++++---
> >  10 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> > index 3ae2357fda54..e2e630f207f0 100644
> > --- a/include/qemu/error-report.h
> > +++ b/include/qemu/error-report.h
> > @@ -13,6 +13,8 @@
> >  #ifndef QEMU_ERROR_REPORT_H
> >  #define QEMU_ERROR_REPORT_H
> >
> > +typedef bool (*ErrorReportDetailedFunc)(void);
> > +
> >  typedef struct Location {
> >      /* all members are private to qemu-error.c */
> >      enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> > @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char
> *fmt, ...)
> >  bool warn_report_once_cond(bool *printed, const char *fmt, ...)
> >      G_GNUC_PRINTF(2, 3);
> >
> > -void error_init(const char *argv0);
> > +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
> >
> >  /*
> >   * Similar to error_report(), except it prints the message just once.
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index 6f09180d6541..d5f8fca863d7 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -292,7 +292,7 @@ int main(int argc, char **argv)
> >
> >      save_proc_pathname(argv[0]);
> >
> > -    error_init(argv[0]);
> > +    error_init(argv[0], NULL);
> >      module_call_init(MODULE_INIT_TRACE);
> >      qemu_init_cpu_list();
> >      module_call_init(MODULE_INIT_QOM);
> [More such calls of error_init()...]
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index 54e920ada1a1..3b46fc9c1fc5 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
> >      }
> >  }
> >
> > +static bool error_is_detailed(void)
> > +{
> > +    return !monitor_cur();
> > +}
> > +
> >  void qemu_init(int argc, char **argv, char **envp)
> >  {
> >      QemuOpts *opts;
> > @@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
> >      qemu_add_opts(&qemu_action_opts);
> >      module_call_init(MODULE_INIT_OPTS);
> >
> > -    error_init(argv[0]);
> > +    error_init(argv[0], error_is_detailed);
> >      qemu_init_exec_dir(argv[0]);
> >
> >      qemu_init_arch_modules();
> > diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> > index c104817cdddc..7e4d5030a045 100644
> > --- a/storage-daemon/qemu-storage-daemon.c
> > +++ b/storage-daemon/qemu-storage-daemon.c
> > @@ -368,13 +368,18 @@ static void pid_file_init(void)
> >      atexit(pid_file_cleanup);
> >  }
> >
> > +static bool error_is_detailed(void)
> > +{
> > +    return !monitor_cur();
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >  #ifdef CONFIG_POSIX
> >      signal(SIGPIPE, SIG_IGN);
> >  #endif
> >
> > -    error_init(argv[0]);
> > +    error_init(argv[0], error_is_detailed);
> >      qemu_init_exec_dir(argv[0]);
> >      os_setup_signal_handling();
> >
> > diff --git a/util/error-report.c b/util/error-report.c
> > index c43227a975e2..c2181f80a83d 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"
> >
> >  /*
> > @@ -28,6 +27,7 @@ typedef enum {
> >  bool message_with_timestamp;
> >  bool error_with_guestname;
> >  const char *error_guest_name;
> > +ErrorReportDetailedFunc detailed_fn = NULL;
> >
> >  int error_printf(const char *fmt, ...)
> >  {
> > @@ -195,7 +195,7 @@ real_time_iso8601(void)
> >   */
> >  static void vreport(report_type type, const char *fmt, va_list ap)
> >  {
> > -    bool detailed = !monitor_cur();
> > +    bool detailed = detailed_fn ? detailed_fn() : TRUE;
> >      gchar *timestr;
> >
> >      if (message_with_timestamp && detailed) {
> > @@ -387,7 +387,7 @@ static void qemu_log_func(const gchar *log_domain,
> >      }
> >  }
> >
> > -void error_init(const char *argv0)
> > +void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
> >  {
> >      const char *p = strrchr(argv0, '/');
> >
> > @@ -401,4 +401,6 @@ void error_init(const char *argv0)
> >      g_log_set_default_handler(qemu_log_func, NULL);
> >      g_warn_if_fail(qemu_glog_domains == NULL);
> >      qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
> > +
> > +    detailed_fn = detailed;
> >  }
>
> A callback works, but note that each program's function is fixed.  Why
> not use the linker to resolve it?  Have a .o in libqemuutil.a that
> defines error_is_detailed() to return false always.  Have another
> error_is_detailed() that returns !monitor_cur() in monitor.c.  A program
> that links the monitor gets the latter, all the others the former.
>
> What do you think?
>

Yes, if we manage to overwrite symbols from a static library in a
subproject. See the other thread for discussion.


-- 
Marc-André Lureau

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

[PATCH 6/9] error-report: add a callback to overwrite error_vprintf

[marcandre.lureau]

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

error_vprintf() is implemented in monitor.c, which overrides the
default implementation from stubs/, while avoiding a direct dependency
to the monitor from error-report.c.

However, the stub solution isn't working when moving error-report.c and
stubs/error-printf.c in a common library. Linking with such library
creates conflicts for the error_vprintf() implementations (and weak
symbols aren't great either). Instead, use the "traditional" approach to
provide overidable callbacks.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/monitor/monitor.h            |  1 +
 include/qemu/error-report.h          |  6 ++++--
 bsd-user/main.c                      |  2 +-
 linux-user/main.c                    |  2 +-
 monitor/monitor.c                    |  2 +-
 qemu-img.c                           |  2 +-
 qemu-io.c                            |  2 +-
 qemu-nbd.c                           |  2 +-
 scsi/qemu-pr-helper.c                |  2 +-
 softmmu/vl.c                         |  2 +-
 storage-daemon/qemu-storage-daemon.c |  2 +-
 stubs/error-printf.c                 | 18 ------------------
 util/error-report.c                  | 27 ++++++++++++++++++++++++---
 stubs/meson.build                    |  1 -
 14 files changed, 38 insertions(+), 33 deletions(-)
 delete mode 100644 stubs/error-printf.c

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 44653e195b45..e94ab2e74889 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,6 +16,7 @@ extern QemuOptsList qemu_mon_opts;
 Monitor *monitor_cur(void);
 Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
 bool monitor_cur_is_qmp(void);
+int monitor_error_vprintf(const char *fmt, va_list ap);
 
 void monitor_init_globals(void);
 void monitor_init_globals_core(void);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e2e630f207f0..a2bc030b4bfe 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,7 @@
 #define QEMU_ERROR_REPORT_H
 
 typedef bool (*ErrorReportDetailedFunc)(void);
+typedef int (*ErrorReportVPrintfFunc)(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
 
 typedef struct Location {
     /* all members are private to qemu-error.c */
@@ -32,7 +33,6 @@ void loc_set_none(void);
 void loc_set_cmdline(char **argv, int idx, int cnt);
 void loc_set_file(const char *fname, int lno);
 
-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);
 
 void error_vreport(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
@@ -48,7 +48,9 @@ bool error_report_once_cond(bool *printed, const char *fmt, ...)
 bool warn_report_once_cond(bool *printed, const char *fmt, ...)
     G_GNUC_PRINTF(2, 3);
 
-void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
+void error_init(const char *argv0,
+                ErrorReportDetailedFunc detailed_fn,
+                ErrorReportVPrintfFunc vprintf_fn);
 
 /*
  * Similar to error_report(), except it prints the message just once.
diff --git a/bsd-user/main.c b/bsd-user/main.c
index d5f8fca863d7..1cc1ba9b2e6e 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -292,7 +292,7 @@ int main(int argc, char **argv)
 
     save_proc_pathname(argv[0]);
 
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_cpu_list();
     module_call_init(MODULE_INIT_QOM);
diff --git a/linux-user/main.c b/linux-user/main.c
index 84f380bd366d..75f72099739d 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -646,7 +646,7 @@ int main(int argc, char **argv, char **envp)
     unsigned long max_reserved_va;
     bool preserve_argv0;
 
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_cpu_list();
     module_call_init(MODULE_INIT_QOM);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index ba4c1716a48a..490e7babd895 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -263,7 +263,7 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
 /*
  * Print to current monitor if we have one, else to stderr.
  */
-int error_vprintf(const char *fmt, va_list ap)
+int monitor_error_vprintf(const char *fmt, va_list ap)
 {
     Monitor *cur_mon = monitor_cur();
 
diff --git a/qemu-img.c b/qemu-img.c
index 1f27a9fc70f6..00383f48f7bc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5396,7 +5396,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
 
diff --git a/qemu-io.c b/qemu-io.c
index b5cdc7c922a7..09794cd781be 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -539,7 +539,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6bc632c93611..112303674cfb 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -587,7 +587,7 @@ int main(int argc, char **argv)
 #endif
 
     socket_init();
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     qcrypto_init(&error_fatal);
 
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 8d80e58d4498..d265d11b6261 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -910,7 +910,7 @@ int main(int argc, char **argv)
 
     signal(SIGPIPE, SIG_IGN);
 
-    error_init(argv[0], NULL);
+    error_init(argv[0], NULL, NULL);
     module_call_init(MODULE_INIT_TRACE);
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_trace_opts);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3b46fc9c1fc5..ef54af0efd6f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2639,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_action_opts);
     module_call_init(MODULE_INIT_OPTS);
 
-    error_init(argv[0], error_is_detailed);
+    error_init(argv[0], error_is_detailed, monitor_error_vprintf);
     qemu_init_exec_dir(argv[0]);
 
     qemu_init_arch_modules();
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 7e4d5030a045..0e0893695628 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -379,7 +379,7 @@ int main(int argc, char *argv[])
     signal(SIGPIPE, SIG_IGN);
 #endif
 
-    error_init(argv[0], error_is_detailed);
+    error_init(argv[0], error_is_detailed, monitor_error_vprintf);
     qemu_init_exec_dir(argv[0]);
     os_setup_signal_handling();
 
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
deleted file mode 100644
index 1afa0f62ca26..000000000000
--- a/stubs/error-printf.c
+++ /dev/null
@@ -1,18 +0,0 @@
-#include "qemu/osdep.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")) {
-        char *msg = g_strdup_vprintf(fmt, ap);
-        g_test_message("%s", msg);
-        ret = strlen(msg);
-        g_free(msg);
-        return ret;
-    }
-    return vfprintf(stderr, fmt, ap);
-}
diff --git a/util/error-report.c b/util/error-report.c
index c2181f80a83d..1452047cd2e8 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -23,11 +23,14 @@ typedef enum {
     REPORT_TYPE_INFO,
 } report_type;
 
+static int error_vprintf(const char *fmt, va_list ap);
+
 /* Prepend timestamp to messages */
 bool message_with_timestamp;
 bool error_with_guestname;
 const char *error_guest_name;
 ErrorReportDetailedFunc detailed_fn = NULL;
+ErrorReportVPrintfFunc vprintf_fn = error_vprintf;
 
 int error_printf(const char *fmt, ...)
 {
@@ -35,7 +38,7 @@ int error_printf(const char *fmt, ...)
     int ret;
 
     va_start(ap, fmt);
-    ret = error_vprintf(fmt, ap);
+    ret = vprintf_fn(fmt, ap);
     va_end(ap);
     return ret;
 }
@@ -222,7 +225,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
         break;
     }
 
-    error_vprintf(fmt, ap);
+    vprintf_fn(fmt, ap);
     error_printf("\n");
 }
 
@@ -387,7 +390,24 @@ static void qemu_log_func(const gchar *log_domain,
     }
 }
 
-void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
+static int error_vprintf(const char *fmt, va_list ap)
+{
+    int ret;
+
+    if (g_test_initialized() && !g_test_subprocess() &&
+        getenv("QTEST_SILENT_ERRORS")) {
+        char *msg = g_strdup_vprintf(fmt, ap);
+        g_test_message("%s", msg);
+        ret = strlen(msg);
+        g_free(msg);
+        return ret;
+    }
+    return vfprintf(stderr, fmt, ap);
+}
+
+void error_init(const char *argv0,
+                ErrorReportDetailedFunc detailed,
+                ErrorReportVPrintfFunc vprintf)
 {
     const char *p = strrchr(argv0, '/');
 
@@ -403,4 +423,5 @@ void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
     qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
 
     detailed_fn = detailed;
+    vprintf_fn = vprintf ?: error_vprintf;
 }
diff --git a/stubs/meson.build b/stubs/meson.build
index d8f3fd5c44f2..498b6ee0466e 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -9,7 +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-printf.c'))
 stub_ss.add(files('fdset.c'))
 stub_ss.add(files('gdbstub.c'))
 stub_ss.add(files('get-vm-name.c'))
-- 
2.37.0.rc0

Re: [PATCH 6/9] error-report: add a callback to overwrite error_vprintf

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> error_vprintf() is implemented in monitor.c, which overrides the
> default implementation from stubs/, while avoiding a direct dependency
> to the monitor from error-report.c.
>
> However, the stub solution isn't working when moving error-report.c and
> stubs/error-printf.c in a common library. Linking with such library
> creates conflicts for the error_vprintf() implementations

I'm feeling dense today: how?

Why would the linker pull in error-printf.o when the symbols it defines
are already provided by .o the linker processed before the library
containing error-printf.o?

>                                                           (and weak
> symbols aren't great either).

Weak symbols are great, except Windows isn't.

>                               Instead, use the "traditional" approach to
> provide overidable callbacks.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 6/9] error-report: add a callback to overwrite error_vprintf

[Marc-André Lureau]

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

Hi

On Thu, Jul 7, 2022 at 4:18 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > error_vprintf() is implemented in monitor.c, which overrides the
> > default implementation from stubs/, while avoiding a direct dependency
> > to the monitor from error-report.c.
> >
> > However, the stub solution isn't working when moving error-report.c and
> > stubs/error-printf.c in a common library. Linking with such library
> > creates conflicts for the error_vprintf() implementations
>
> I'm feeling dense today: how?
>

If I try to overwrite a symbol in qemu when linking with a static library
from a subproject, the linker fails:

FAILED: storage-daemon/qemu-storage-daemon
cc -m64 -mcx16  -o storage-daemon/qemu-storage-daemon
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-introspect.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-types.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-visit.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-commands.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-init-commands.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-events.c.o
storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-emit-events.c.o
storage-daemon/qemu-storage-daemon.p/qemu-storage-daemon.c.o
-Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libblockdev.fa
libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa -Wl,--start-group
libevent-loop-base.a libchardev.fa libqmp.fa -Wl,--no-whole-archive
-Wl,--warn-common -Wl,-z,relro -Wl,-z,now -fstack-protector-strong
-Wl,-rpath,/usr/lib64/iscsi -Wl,-rpath-link,/usr/lib64/iscsi libqemuutil.a
subprojects/libvhost-user/libvhost-user-glib.a
subprojects/libvhost-user/libvhost-user.a
subprojects/qemu-common/libqemu-common.a libblockdev.fa
subprojects/libvduse/libvduse.a libblock.fa libcrypto.fa libauthz.fa
libqom.fa libio.fa libchardev.fa libqmp.fa @block.syms
/usr/lib64/libgnutls.so /usr/lib64/liburing.so /usr/lib64/libgio-2.0.so
/usr/lib64/libgobject-2.0.so /usr/lib64/libglib-2.0.so -Wl,--export-dynamic
-pthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lm /usr/lib64/libpixman-1.so
-lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lgmodule-2.0 -lglib-2.0 -lglib-2.0
-lgmodule-2.0 -lglib-2.0 -lglib-2.0 /usr/lib64/libfuse3.so -lpthread
-lgmodule-2.0 -lglib-2.0 -lglib-2.0 /usr/lib64/libzstd.so
/usr/lib64/libz.so /usr/lib64/iscsi/libiscsi.so -laio -lpam -lutil
-Wl,--end-group
/usr/bin/ld:
subprojects/qemu-common/libqemu-common.a.p/src_error-report.c.o: in
function `error_init':
/home/elmarco/src/qemu/build/../subprojects/qemu-common/src/error-report.c:413:
multiple definition of `error_init';
libqmp.fa.p/monitor_qmp.c.o:/home/elmarco/src/qemu/build/../monitor/qmp.c:40:
first defined here

But I can't really explain why it works when overwriting symbols from
libqemuutil.a, I am a bit puzzled here. Am I missing something obvious?
(this is a bit of dark linker magic going on)



> Why would the linker pull in error-printf.o when the symbols it defines
> are already provided by .o the linker processed before the library
> containing error-printf.o?
>
> >                                                           (and weak
> > symbols aren't great either).
>
> Weak symbols are great, except Windows isn't.
>
> >                               Instead, use the "traditional" approach to
> > provide overidable callbacks.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
>

-- 
Marc-André Lureau

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

Re: [PATCH 6/9] error-report: add a callback to overwrite error_vprintf

[Marc-André Lureau]

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

Hi

On Thu, Jul 7, 2022 at 10:05 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Thu, Jul 7, 2022 at 4:18 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > error_vprintf() is implemented in monitor.c, which overrides the
>> > default implementation from stubs/, while avoiding a direct dependency
>> > to the monitor from error-report.c.
>> >
>> > However, the stub solution isn't working when moving error-report.c and
>> > stubs/error-printf.c in a common library. Linking with such library
>> > creates conflicts for the error_vprintf() implementations
>>
>> I'm feeling dense today: how?
>>
>
> If I try to overwrite a symbol in qemu when linking with a static library
> from a subproject, the linker fails:
>
> FAILED: storage-daemon/qemu-storage-daemon
> cc -m64 -mcx16  -o storage-daemon/qemu-storage-daemon
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-introspect.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-types.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-visit.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-commands.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-init-commands.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-events.c.o
> storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-emit-events.c.o
> storage-daemon/qemu-storage-daemon.p/qemu-storage-daemon.c.o
> -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libblockdev.fa
> libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa -Wl,--start-group
> libevent-loop-base.a libchardev.fa libqmp.fa -Wl,--no-whole-archive
> -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -fstack-protector-strong
> -Wl,-rpath,/usr/lib64/iscsi -Wl,-rpath-link,/usr/lib64/iscsi libqemuutil.a
> subprojects/libvhost-user/libvhost-user-glib.a
> subprojects/libvhost-user/libvhost-user.a
> subprojects/qemu-common/libqemu-common.a libblockdev.fa
> subprojects/libvduse/libvduse.a libblock.fa libcrypto.fa libauthz.fa
> libqom.fa libio.fa libchardev.fa libqmp.fa @block.syms
> /usr/lib64/libgnutls.so /usr/lib64/liburing.so /usr/lib64/libgio-2.0.so
> /usr/lib64/libgobject-2.0.so /usr/lib64/libglib-2.0.so
> -Wl,--export-dynamic -pthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lm
> /usr/lib64/libpixman-1.so -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lgmodule-2.0
> -lglib-2.0 -lglib-2.0 -lgmodule-2.0 -lglib-2.0 -lglib-2.0
> /usr/lib64/libfuse3.so -lpthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0
> /usr/lib64/libzstd.so /usr/lib64/libz.so /usr/lib64/iscsi/libiscsi.so -laio
> -lpam -lutil -Wl,--end-group
> /usr/bin/ld:
> subprojects/qemu-common/libqemu-common.a.p/src_error-report.c.o: in
> function `error_init':
> /home/elmarco/src/qemu/build/../subprojects/qemu-common/src/error-report.c:413:
> multiple definition of `error_init';
> libqmp.fa.p/monitor_qmp.c.o:/home/elmarco/src/qemu/build/../monitor/qmp.c:40:
> first defined here
>
> But I can't really explain why it works when overwriting symbols from
> libqemuutil.a, I am a bit puzzled here. Am I missing something obvious?
> (this is a bit of dark linker magic going on)
>
>
After playing with this for a few hours ... I managed to get the symbol
override work, by having a single overridable function per unit. No idea
why in qemutil.a/stubs we manage to have several. That's a mystery. Anyway,
I will send a new version where I also introduce the subproject, to
demonstrate it works.

thanks


>
>> Why would the linker pull in error-printf.o when the symbols it defines
>> are already provided by .o the linker processed before the library
>> containing error-printf.o?
>>
>> >                                                           (and weak
>> > symbols aren't great either).
>>
>> Weak symbols are great, except Windows isn't.
>>
>> >                               Instead, use the "traditional" approach to
>> > provide overidable callbacks.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>
>>
>
> --
> Marc-André Lureau
>


-- 
Marc-André Lureau

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

[PATCH 7/9] qapi: move QEMU-specific dispatch code in monitor

[marcandre.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

[PATCH 8/9] scripts/qapi-gen: add -i option

[marcandre.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

Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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>

I wish we'd all agree on "config.h" (conventional with autoconf).  But
we don't.

Precedence for tweaking generated code with command line options: -p.

I suspect that we'll always specify the same -p and -i for a given
schema.  To me, that suggests they should both go into the schema
instead.  Observation, not demand.

> ---
>  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):

Ignorant question: why ist this List[str], not str?  Do multiple options
accumulate into a list?

Alright, I'm back from further down: looks like they do.

>          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])

This maps a list of file names / #include arguments to C code including
them, mapping file names to #include arguments by enclosing them in "".

Option arguments of the form <foo.h> and "foo.h" need to be quoted in
the shell.  The latter can be done without quoting as foo.h.

Somewhat funky wedding of generality with convenience.

> +
> +
>  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")

The option name --include doesn't really tell me what it is about.  Is
it an include path for schema files?  Or is it about including something
in generated C?  Where in generated C?

The help text provides clues: "headers" suggests .h, and "top-level"
suggests somewhere top-like.

In fact, it's about inserting C code at the beginning of generated .c
files.  For the uses we have in mind, the C code is a single #include.
The patch implements any number of #includes.

More general and arguably less funky: a way to insert arbitrary C code.

Except arbitrary C code on the command line is unwieldy.  Better kept it
in the schema.  Pragma?

Thoughts?

>  
>      # 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)

Patch is mostly plumbing.  Looks reasonable.

Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Marc-André Lureau]

Hi


On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > 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>
>
> I wish we'd all agree on "config.h" (conventional with autoconf).  But
> we don't.
>
> Precedence for tweaking generated code with command line options: -p.
>
> I suspect that we'll always specify the same -p and -i for a given
> schema.  To me, that suggests they should both go into the schema
> instead.  Observation, not demand.
>
> > ---
> >  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):
>
> Ignorant question: why ist this List[str], not str?  Do multiple options
> accumulate into a list?
>
> Alright, I'm back from further down: looks like they do.
>
> >          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])
>
> This maps a list of file names / #include arguments to C code including
> them, mapping file names to #include arguments by enclosing them in "".
>
> Option arguments of the form <foo.h> and "foo.h" need to be quoted in
> the shell.  The latter can be done without quoting as foo.h.
>
> Somewhat funky wedding of generality with convenience.
>
> > +
> > +
> >  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")
>
> The option name --include doesn't really tell me what it is about.  Is
> it an include path for schema files?  Or is it about including something
> in generated C?  Where in generated C?
>
> The help text provides clues: "headers" suggests .h, and "top-level"
> suggests somewhere top-like.
>
> In fact, it's about inserting C code at the beginning of generated .c
> files.  For the uses we have in mind, the C code is a single #include.
> The patch implements any number of #includes.
>
> More general and arguably less funky: a way to insert arbitrary C code.
>
> Except arbitrary C code on the command line is unwieldy.  Better kept it
> in the schema.  Pragma?
>
> Thoughts?

Pragmas are global currently. This doesn't scale well, as we would
like to split the schemas. I have a following patch that will allow me
to split/merge the pragmas. This is not optimal either, I would rather
remove/replace them (using annotations).

Imho, global tweaking of compilation is better done from the command line.

>
> >
> >      # 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)
>
> Patch is mostly plumbing.  Looks reasonable.
>

Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Markus Armbruster]

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

> Hi
>
>
> On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > 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>
>>
>> I wish we'd all agree on "config.h" (conventional with autoconf).  But
>> we don't.
>>
>> Precedence for tweaking generated code with command line options: -p.
>>
>> I suspect that we'll always specify the same -p and -i for a given
>> schema.  To me, that suggests they should both go into the schema
>> instead.  Observation, not demand.
>>
>> > ---
>> >  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):
>>
>> Ignorant question: why ist this List[str], not str?  Do multiple options
>> accumulate into a list?
>>
>> Alright, I'm back from further down: looks like they do.
>>
>> >          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])
>>
>> This maps a list of file names / #include arguments to C code including
>> them, mapping file names to #include arguments by enclosing them in "".
>>
>> Option arguments of the form <foo.h> and "foo.h" need to be quoted in
>> the shell.  The latter can be done without quoting as foo.h.
>>
>> Somewhat funky wedding of generality with convenience.
>>
>> > +
>> > +
>> >  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")
>>
>> The option name --include doesn't really tell me what it is about.  Is
>> it an include path for schema files?  Or is it about including something
>> in generated C?  Where in generated C?
>>
>> The help text provides clues: "headers" suggests .h, and "top-level"
>> suggests somewhere top-like.
>>
>> In fact, it's about inserting C code at the beginning of generated .c
>> files.  For the uses we have in mind, the C code is a single #include.
>> The patch implements any number of #includes.
>>
>> More general and arguably less funky: a way to insert arbitrary C code.
>>
>> Except arbitrary C code on the command line is unwieldy.  Better kept it
>> in the schema.  Pragma?
>>
>> Thoughts?
>
> Pragmas are global currently. This doesn't scale well, as we would
> like to split the schemas. I have a following patch that will allow me
> to split/merge the pragmas. This is not optimal either, I would rather
> remove/replace them (using annotations).

Now I'm curious.  Can you sketch what you have in mind?

> Imho, global tweaking of compilation is better done from the command line.

The command line is fine for straightforward configuration.  It's not
suitable for injecting code.

Fine: cc -c, which tells the compiler to work in a certain way.

Still fine: cc -DFOO, which effectively prepends '#define FOO 1" to the
.c.

No longer fine: a hypothetical option to prepend arbitrary C code.  Even
if it was occasionally useful.

Now watch this:

    $ python qapi-gen.py -o t qapi/qapi-schema.json -i '"abc.h"
    #define FOO'

    $ head -n 16 t/qapi-types.c
    /* AUTOMATICALLY GENERATED, DO NOT MODIFY */

    /*
     * Schema-defined QAPI types
     *
     * Copyright IBM, Corp. 2011
     * Copyright (c) 2013-2018 Red Hat Inc.
     *
     * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
     * See the COPYING.LIB file in the top-level directory.
     */

    #include "abc.h"
    #define FOO

    #include "qapi/dealloc-visitor.h"

Sure, nobody of sane mind would ever do this.  The fact remains that
we're doing something on the command line that should not be done there.

Your -i enables code injection because it takes either a file name or a
#include argument.  Can we dumb it down to just file name?

[...]

Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Marc-André Lureau]

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

Hi

On Tue, Aug 2, 2022 at 5:28 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> >
> >
> > On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > 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>
> >>
> >> I wish we'd all agree on "config.h" (conventional with autoconf).  But
> >> we don't.
> >>
> >> Precedence for tweaking generated code with command line options: -p.
> >>
> >> I suspect that we'll always specify the same -p and -i for a given
> >> schema.  To me, that suggests they should both go into the schema
> >> instead.  Observation, not demand.
> >>
> >> > ---
> >> >  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):
> >>
> >> Ignorant question: why ist this List[str], not str?  Do multiple options
> >> accumulate into a list?
> >>
> >> Alright, I'm back from further down: looks like they do.
> >>
> >> >          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])
> >>
> >> This maps a list of file names / #include arguments to C code including
> >> them, mapping file names to #include arguments by enclosing them in "".
> >>
> >> Option arguments of the form <foo.h> and "foo.h" need to be quoted in
> >> the shell.  The latter can be done without quoting as foo.h.
> >>
> >> Somewhat funky wedding of generality with convenience.
> >>
> >> > +
> >> > +
> >> >  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")
> >>
> >> The option name --include doesn't really tell me what it is about.  Is
> >> it an include path for schema files?  Or is it about including something
> >> in generated C?  Where in generated C?
> >>
> >> The help text provides clues: "headers" suggests .h, and "top-level"
> >> suggests somewhere top-like.
> >>
> >> In fact, it's about inserting C code at the beginning of generated .c
> >> files.  For the uses we have in mind, the C code is a single #include.
> >> The patch implements any number of #includes.
> >>
> >> More general and arguably less funky: a way to insert arbitrary C code.
> >>
> >> Except arbitrary C code on the command line is unwieldy.  Better kept it
> >> in the schema.  Pragma?
> >>
> >> Thoughts?
> >
> > Pragmas are global currently. This doesn't scale well, as we would
> > like to split the schemas. I have a following patch that will allow me
> > to split/merge the pragmas. This is not optimal either, I would rather
> > remove/replace them (using annotations).
>
> Now I'm curious.  Can you sketch what you have in mind?
>

I simply made the pragma lists additive:

https://gitlab.com/marcandre.lureau/qemu/-/commit/1861964a317c2e74bea2d1f86944625e00df777f


I didn't think much about replacing pragmas with extra annotations. But it
could be for ex moving some pragmas to the declarations.

From:

{ 'pragma': {
    # Command names containing '_'
    'command-name-exceptions': [
        'add_client',
...

{ 'command': 'add_client',
  'data': { ... } }

To:

{ 'command': {
    'name': 'add_client',
    # Command name containing '_'
    'name-exception': true },
  'data': { ... } }

Or eventually to the comment:

# @add_client: (name-exception):



> > Imho, global tweaking of compilation is better done from the command
> line.
>
> The command line is fine for straightforward configuration.  It's not
> suitable for injecting code.
>
> Fine: cc -c, which tells the compiler to work in a certain way.
>
> Still fine: cc -DFOO, which effectively prepends '#define FOO 1" to the
> .c.
>
> No longer fine: a hypothetical option to prepend arbitrary C code.  Even
> if it was occasionally useful.
>
> Now watch this:
>
>     $ python qapi-gen.py -o t qapi/qapi-schema.json -i '"abc.h"
>     #define FOO'
>
>     $ head -n 16 t/qapi-types.c
>     /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>
>     /*
>      * Schema-defined QAPI types
>      *
>      * Copyright IBM, Corp. 2011
>      * Copyright (c) 2013-2018 Red Hat Inc.
>      *
>      * This work is licensed under the terms of the GNU LGPL, version 2.1
> or later.
>      * See the COPYING.LIB file in the top-level directory.
>      */
>
>     #include "abc.h"
>     #define FOO
>
>     #include "qapi/dealloc-visitor.h"
>
> Sure, nobody of sane mind would ever do this.  The fact remains that
> we're doing something on the command line that should not be done there.
>
> Your -i enables code injection because it takes either a file name or a
> #include argument.  Can we dumb it down to just file name?
>
>
I think that can work too. Users can include a header that itself includes
extra headers in different ways, if needed.

thanks

-- 
Marc-André Lureau

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

Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Aug 2, 2022 at 5:28 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi
>> >
>> >
>> > On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> >> The option name --include doesn't really tell me what it is about.  Is
>> >> it an include path for schema files?  Or is it about including something
>> >> in generated C?  Where in generated C?
>> >>
>> >> The help text provides clues: "headers" suggests .h, and "top-level"
>> >> suggests somewhere top-like.
>> >>
>> >> In fact, it's about inserting C code at the beginning of generated .c
>> >> files.  For the uses we have in mind, the C code is a single #include.
>> >> The patch implements any number of #includes.
>> >>
>> >> More general and arguably less funky: a way to insert arbitrary C code.
>> >>
>> >> Except arbitrary C code on the command line is unwieldy.  Better kept it
>> >> in the schema.  Pragma?
>> >>
>> >> Thoughts?
>> >
>> > Pragmas are global currently. This doesn't scale well, as we would
>> > like to split the schemas. I have a following patch that will allow me
>> > to split/merge the pragmas. This is not optimal either, I would rather
>> > remove/replace them (using annotations).
>>
>> Now I'm curious.  Can you sketch what you have in mind?
>>
>
> I simply made the pragma lists additive:
>
> https://gitlab.com/marcandre.lureau/qemu/-/commit/1861964a317c2e74bea2d1f86944625e00df777f
>
>
> I didn't think much about replacing pragmas with extra annotations. But it
> could be for ex moving some pragmas to the declarations.
>
> From:
>
> { 'pragma': {
>     # Command names containing '_'
>     'command-name-exceptions': [
>         'add_client',
> ...
>
> { 'command': 'add_client',
>   'data': { ... } }
>
> To:
>
> { 'command': {
>     'name': 'add_client',
>     # Command name containing '_'
>     'name-exception': true },
>   'data': { ... } }
>
> Or eventually to the comment:
>
> # @add_client: (name-exception):

Keeping the QAPI rule violation overrides separate is kind of awkward,
but 1. it makes rule violations easy to spot in review, and 2. making
rule violations awkward helps deter people from violating the rules.

I figure the point of making pragmas additive is to let us avoid
duplication as we go from single schema to multiple schemas sharing
stuff.

We already do that for the storage daemon, admittedly in a crude &
stupid way.  We simply reuse the entire pragma.json.  Possible because
unused ones get ignored.

>> > Imho, global tweaking of compilation is better done from the command
>> > line.
>>
>> The command line is fine for straightforward configuration.  It's not
>> suitable for injecting code.
>>
>> Fine: cc -c, which tells the compiler to work in a certain way.
>>
>> Still fine: cc -DFOO, which effectively prepends '#define FOO 1" to the
>> .c.
>>
>> No longer fine: a hypothetical option to prepend arbitrary C code.  Even
>> if it was occasionally useful.
>>
>> Now watch this:
>>
>>     $ python qapi-gen.py -o t qapi/qapi-schema.json -i '"abc.h"
>>     #define FOO'
>>
>>     $ head -n 16 t/qapi-types.c
>>     /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>>
>>     /*
>>      * Schema-defined QAPI types
>>      *
>>      * Copyright IBM, Corp. 2011
>>      * Copyright (c) 2013-2018 Red Hat Inc.
>>      *
>>      * This work is licensed under the terms of the GNU LGPL, version 2.1
>> or later.
>>      * See the COPYING.LIB file in the top-level directory.
>>      */
>>
>>     #include "abc.h"
>>     #define FOO
>>
>>     #include "qapi/dealloc-visitor.h"
>>
>> Sure, nobody of sane mind would ever do this.  The fact remains that
>> we're doing something on the command line that should not be done there.
>>
>> Your -i enables code injection because it takes either a file name or a
>> #include argument.  Can we dumb it down to just file name?
>>
>>
> I think that can work too. Users can include a header that itself includes
> extra headers in different ways, if needed.

Yes.  It could even be named "qemu/osdep.h" ;)

Teasing aside, I'm okay with a simple option to override the name of the
header to include first.

[PATCH 9/9] scripts/qapi: add required system includes to visitor

[marcandre.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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 1ff464c0360f..d686df17f4b6 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -326,6 +326,8 @@ def __init__(self, prefix: str, include: List[str]):
     def _begin_builtin_module(self) -> None:
         self._genc.preamble_add(mcgen('''
 %(include)s
+#include <assert.h>
+#include <stdlib.h>
 
 #include "qapi/error.h"
 #include "qapi/qapi-builtin-visit.h"
@@ -342,6 +344,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

Re: [PATCH 9/9] scripts/qapi: add required system includes to visitor

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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.

Suggest ", even though the default qemu/osdep.h always does.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/visit.py | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 1ff464c0360f..d686df17f4b6 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -326,6 +326,8 @@ def __init__(self, prefix: str, include: List[str]):
>      def _begin_builtin_module(self) -> None:
>          self._genc.preamble_add(mcgen('''
>  %(include)s
> +#include <assert.h>
> +#include <stdlib.h>
>  
>  #include "qapi/error.h"
>  #include "qapi/qapi-builtin-visit.h"
> @@ -342,6 +344,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"

Mildly irritating, because we normally kill such includes as redundant
on sight.

The builtin module (qapi-builtin-visit.c) doesn't actually need these
headers.  I guess you include them just in case that changes.

Re: [PATCH 9/9] scripts/qapi: add required system includes to visitor

[Marc-André Lureau]

Hi

On Thu, Jun 23, 2022 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > 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.
>
> Suggest ", even though the default qemu/osdep.h always does.
>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/qapi/visit.py | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 1ff464c0360f..d686df17f4b6 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -326,6 +326,8 @@ def __init__(self, prefix: str, include: List[str]):
> >      def _begin_builtin_module(self) -> None:
> >          self._genc.preamble_add(mcgen('''
> >  %(include)s
> > +#include <assert.h>
> > +#include <stdlib.h>
> >
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-builtin-visit.h"
> > @@ -342,6 +344,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"
>
> Mildly irritating, because we normally kill such includes as redundant
> on sight.
>
> The builtin module (qapi-builtin-visit.c) doesn't actually need these
> headers.  I guess you include them just in case that changes.

True, at least not directly. I will drop it for now, we can add it
back when/if I figure out it is necessary.

Re: [PATCH 0/9] Preliminary patches for subproject split

[Paolo Bonzini]

On 6/16/22 14:40, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Here is another subset of the large "subproject(qga)"" series I intend to send
> soon after (https://gitlab.com/marcandre.lureau/qemu/-/commits/qga).

Hi,

I took a peek there and I have a question.  While the configure script 
has had enough of a diet that it should be possible to compile the 
subprojects as standalone with relatively little effort, how do you plan 
to handle things such as compiler flags that are detected by meson.build 
(especially enabling/disabling warnings)?

Paolo

> Thanks
> 
> Marc-André Lureau (9):
>    monitor: make error_vprintf_unless_qmp() static
>    error-report: misc comment fix
>    error-report: introduce "detailed" variable
>    error-report: simplify print_loc()
>    error-report: introduce ErrorReportDetailedFunc
>    error-report: add a callback to overwrite error_vprintf
>    qapi: move QEMU-specific dispatch code in monitor
>    scripts/qapi-gen: add -i option
>    scripts/qapi: add required system includes to visitor
> 
>   include/monitor/monitor.h            |  2 +-
>   include/qapi/qmp/dispatch.h          |  7 ++-
>   include/qemu/error-report.h          |  8 +++-
>   bsd-user/main.c                      |  2 +-
>   linux-user/main.c                    |  2 +-
>   monitor/monitor.c                    |  5 +-
>   monitor/qmp.c                        | 68 +++++++++++++++++++++++++++-
>   qapi/qmp-dispatch.c                  | 64 ++------------------------
>   qemu-img.c                           |  2 +-
>   qemu-io.c                            |  2 +-
>   qemu-nbd.c                           |  2 +-
>   qga/main.c                           |  2 +-
>   scsi/qemu-pr-helper.c                |  2 +-
>   softmmu/vl.c                         |  7 ++-
>   storage-daemon/qemu-storage-daemon.c |  7 ++-
>   stubs/error-printf.c                 | 23 ----------
>   tests/unit/test-qmp-cmds.c           |  6 +--
>   util/error-report.c                  | 46 ++++++++++++++-----
>   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                | 21 ++++++---
>   stubs/meson.build                    |  1 -
>   26 files changed, 226 insertions(+), 147 deletions(-)
>   delete mode 100644 stubs/error-printf.c
> 

Re: [PATCH 0/9] Preliminary patches for subproject split

[Marc-André Lureau]

Hi

On Thu, Jun 16, 2022 at 5:12 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 6/16/22 14:40, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Hi,
> >
> > Here is another subset of the large "subproject(qga)"" series I intend to send
> > soon after (https://gitlab.com/marcandre.lureau/qemu/-/commits/qga).
>
> Hi,
>
> I took a peek there and I have a question.  While the configure script
> has had enough of a diet that it should be possible to compile the
> subprojects as standalone with relatively little effort, how do you plan
> to handle things such as compiler flags that are detected by meson.build
> (especially enabling/disabling warnings)?
>

If you run top-level qemu configure, then I think flags should be
shared with add_global_arguments() (to be verified).

qemu-ga (and qemu-common) in the branch do compile standalone. There
are still small quirks to fix the CI though with the last patches.
Also the last patch could probably deserve a little bit more splitting
(for ex, switching from QEMU_FULL_VERSION to VCS_TAG etc), that branch
is still WIP (after 2 months!).


> Paolo
>
> > Thanks
> >
> > Marc-André Lureau (9):
> >    monitor: make error_vprintf_unless_qmp() static
> >    error-report: misc comment fix
> >    error-report: introduce "detailed" variable
> >    error-report: simplify print_loc()
> >    error-report: introduce ErrorReportDetailedFunc
> >    error-report: add a callback to overwrite error_vprintf
> >    qapi: move QEMU-specific dispatch code in monitor
> >    scripts/qapi-gen: add -i option
> >    scripts/qapi: add required system includes to visitor
> >
> >   include/monitor/monitor.h            |  2 +-
> >   include/qapi/qmp/dispatch.h          |  7 ++-
> >   include/qemu/error-report.h          |  8 +++-
> >   bsd-user/main.c                      |  2 +-
> >   linux-user/main.c                    |  2 +-
> >   monitor/monitor.c                    |  5 +-
> >   monitor/qmp.c                        | 68 +++++++++++++++++++++++++++-
> >   qapi/qmp-dispatch.c                  | 64 ++------------------------
> >   qemu-img.c                           |  2 +-
> >   qemu-io.c                            |  2 +-
> >   qemu-nbd.c                           |  2 +-
> >   qga/main.c                           |  2 +-
> >   scsi/qemu-pr-helper.c                |  2 +-
> >   softmmu/vl.c                         |  7 ++-
> >   storage-daemon/qemu-storage-daemon.c |  7 ++-
> >   stubs/error-printf.c                 | 23 ----------
> >   tests/unit/test-qmp-cmds.c           |  6 +--
> >   util/error-report.c                  | 46 ++++++++++++++-----
> >   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                | 21 ++++++---
> >   stubs/meson.build                    |  1 -
> >   26 files changed, 226 insertions(+), 147 deletions(-)
> >   delete mode 100644 stubs/error-printf.c
> >
>

Re: [PATCH 0/9] Preliminary patches for subproject split

[Marc-André Lureau]

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

Hi Markus

On Thu, Jun 16, 2022 at 4:48 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> Here is another subset of the large "subproject(qga)"" series I intend to
> send
> soon after (https://gitlab.com/marcandre.lureau/qemu/-/commits/qga).
>
> Thanks
>
> Marc-André Lureau (9):
>   monitor: make error_vprintf_unless_qmp() static
>   error-report: misc comment fix
>   error-report: introduce "detailed" variable
>   error-report: simplify print_loc()
>   error-report: introduce ErrorReportDetailedFunc
>   error-report: add a callback to overwrite error_vprintf
>   qapi: move QEMU-specific dispatch code in monitor
>   scripts/qapi-gen: add -i option
>   scripts/qapi: add required system includes to visitor
>

This is mostly your area of maintenance. Do you have time for the remaining
reviews? (The progress feels very slow, compared to what is left in the
queue, I'd like to flush this before next year!)

thanks

-- 
Marc-André Lureau

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