| Message ID | 20240320020115.18801-1-yaoxt.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | contrib/plugins/execlog: Fix compiler warning | expand |
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> > --- > 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 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); > } thanks -- PMM
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
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
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
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) 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 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); }
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); | ~~~~~~~~~~~~~~~~~~^~~~ Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> --- contrib/plugins/execlog.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)