RE: [PATCH] contrib/plugins/execlog: Fix compiler warning

[Xingtao Yao (Fujitsu)]

Pete:
Thanks for your comment.

I also find a similar patch written by Pierrick: 
https://lore.kernel.org/qemu-devel/20240118032400.3762658-15-pierrick.bouvier@linaro.org/
but for some reason, the patch was not merged yet.

shall I need to continue tracking the fixes of this bug?

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Friday, March 22, 2024 7:50 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: alex.bennee@linaro.org; erdnaxe@crans.org; ma.mandourr@gmail.com;
> pierrick.bouvier@linaro.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
> 
> On Wed, 20 Mar 2024 at 02:05, Yao Xingtao via <qemu-devel@nongnu.org>
> wrote:
> >
> > 1. The g_pattern_match_string() is deprecated when glib2 version >= 2.70.
> >    Use g_pattern_spec_match_string() instead to avoid this problem.
> >
> > 2. The type of second parameter in g_ptr_array_add() is
> >    'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
> >    Cast the type of reg->name to 'gpointer' to avoid this problem.
> >
> > compiler warning message:
> > /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> >   330 |                 if (g_pattern_match_string(pat, rd->name) ||
> >       |                 ^~
> > In file included from /usr/include/glib-2.0/glib.h:67,
> >                  from /root/qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >    57 | gboolean      g_pattern_match_string   (GPatternSpec *pspec,
> >       |               ^~~~~~~~~~~~~~~~~~~~~~
> > /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> >   331 |                     g_pattern_match_string(pat, rd_lower)) {
> >       |                     ^~~~~~~~~~~~~~~~~~~~~~
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >    57 | gboolean      g_pattern_match_string   (GPatternSpec *pspec,
> >       |               ^~~~~~~~~~~~~~~~~~~~~~
> > /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing argument
> > 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target
> > type [-Wdiscarded-qualifiers]
> >   339 |                             g_ptr_array_add(all_reg_names,
> reg->name);
> >       |
> ~~~^~~~~~
> > In file included from /usr/include/glib-2.0/glib.h:33:
> > /usr/include/glib-2.0/glib/garray.h:198:62: note: expected ‘gpointer’
> > {aka ‘void *’} but argument is of type ‘const char *’
> >   198 |                                            gpointer
> data);
> >       |
> ~~~~~~~~~~~~~~~~~~^~~~
> >
> 
> Hi; thanks for this patch.
> 
> This fixes a bug reported in the bug tracker so we should put
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
> 
> in the commit message just above your signed-off-by tag.
> 
> 
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
I will if needed.

> > ---
> >  contrib/plugins/execlog.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> > index a1dfd59ab7..41f6774116 100644
> > --- a/contrib/plugins/execlog.c
> > +++ b/contrib/plugins/execlog.c
> > @@ -327,8 +327,13 @@ static GPtrArray *registers_init(int vcpu_index)
> >              for (int p = 0; p < rmatches->len; p++) {
> >                  g_autoptr(GPatternSpec) pat =
> g_pattern_spec_new(rmatches->pdata[p]);
> >                  g_autofree gchar *rd_lower = g_utf8_strdown(rd->name,
> > -1);
> > +#if GLIB_VERSION_MAX_ALLOWED < G_ENCODE_VERSION(2, 70)
> 
> Elsewhere we do glib version checks with
> 
> #if GLIB_CHECK_VERSION(2, 70, 0)
>     code for 2.70.0 and up;
> #else
>     code for older versions
> #endif
> 
> so I think we should probably do that here too.
> 
> >                  if (g_pattern_match_string(pat, rd->name) ||
> >                      g_pattern_match_string(pat, rd_lower)) {
> > +#else
> > +                if (g_pattern_spec_match_string(pat, rd->name) ||
> > +                    g_pattern_spec_match_string(pat, rd_lower)) {
> > +#endif
thanks, I got it.

> 
> Rather than putting this ifdef in the middle of this function, I think it 
> would be
> easier to read if we abstract out a function which does the pattern matching
> and whose body calls the right glib function depending on glib version. We
> generally call these functions the same as the glib function but with a _qemu
> suffix (compare the ones in include/glib-compat.h), so here that would be
> g_pattern_spec_match_string_qemu().
> 
> >                      Register *reg = init_vcpu_register(rd);
> >                      g_ptr_array_add(registers, reg);
> >
> > @@ -336,7 +341,7 @@ static GPtrArray *registers_init(int vcpu_index)
> >                      if (disas_assist) {
> >                          g_mutex_lock(&add_reg_name_lock);
> >                          if (!g_ptr_array_find(all_reg_names, reg->name,
> NULL)) {
> > -                            g_ptr_array_add(all_reg_names, reg->name);
> > +                            g_ptr_array_add(all_reg_names,
> > + (gpointer)reg->name);
> >                          }
> >                          g_mutex_unlock(&add_reg_name_lock);
> >                      }
> 
I think it's not worth adding this to glib-compat.h too.

> thanks
> -- PMM

thanks
Xingtao