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

[Xingtao Yao (Fujitsu)]

Hi Pierrick,

thanks for your reply,  and I will modify and push the patch later.

thanks
Xingtao

> -----Original Message-----
> From: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Sent: Monday, March 25, 2024 12:31 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; Peter Maydell
> <peter.maydell@linaro.org>
> Cc: alex.bennee@linaro.org; erdnaxe@crans.org; ma.mandourr@gmail.com;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
> 
> On 3/25/24 07:00, Xingtao Yao (Fujitsu) wrote:
> > 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?
> >
> 
> Hi Xingtao,
> 
> you're doing the right thing here. In my original patch, there was no
> consideration for backward compatibility, to the opposite of what you did.
> 
> Alex will be out for several weeks, so it might take some time to get this 
> merged,
> but I'm definitely voting for this 👍.
> 
> Pierrick
> 
> >> -----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