On Fri, Mar 01, 2024 at 10:22:08AM +0000, Alex Bennée wrote:
> Date: Fri, 01 Mar 2024 10:22:08 +0000
> From: Alex Bennée <alex.ben...@linaro.org>
> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register
> changes
>
> Zhao Liu <zhao1....@intel.com> writes:
>
> > Hi Alex,
> >
> > I hit the following warnings (with "./configure --enable-werror"):
> >
> > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’:
> > /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:65,
> > from /qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here
> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > /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)) {
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > In file included from /usr/include/glib-2.0/glib.h:65,
> > from /qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here
> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > /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);
>
> Hmm I missed that. Not sure what the neatest solution is in this case -
> g_ptr_array_new() doesn't have a destroy func so we shouldn't ever
> attempt to free it. We can manually re-add the const qualifier at the
> other end for completeness and I guess comment and cast?
I find other palces use 2 ways:
* Use g_strdup() to create a copy (e.g., net/net.c,
add_nic_model_help()). But I'm not sure if this is OK since you said
we shouldn't attempt to free it. May I ask if the free issue you
mentioned will affect the use of g_strdup() here?
* Another way is the forced conversion to gpointer (also e.g., in
net/net.c, qemu_get_nic_models()).
Which way do you like? ;-)
>
>
> > | ~~~^~~~~~
> > In file included from /usr/include/glib-2.0/glib.h:31,
> > from /qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/garray.h:192:62: note: expected ‘gpointer’ {aka
> > ‘void *’} but argument is of type ‘const char *’
> > 192 | gpointer data);
> > | ~~~~~~~~~~~~~~~~~~^~~~
> >
> > In addition, I checked my glic version:
> >
> > $ldd --version
> > ldd (Ubuntu GLIBC 2.35-0ubuntu3.5) 2.35
> >
> > I think it's v2.35. Are these three warning reports valid?
>
> It's the glib (not glibc) version that matters here.
> g_pattern_match_string was deprecated in 2.70 when the suggested
> alternative was added. However our baseline for glib is still:
>
> # When bumping glib minimum version, please check also whether to increase
> # the _WIN32_WINNT setting in osdep.h according to the value from glib
> glib_req_ver = '>=2.56.0'
> glib_pc = dependency('glib-2.0', version: glib_req_ver, required: true,
> method: 'pkg-config')
>
> The usual solution for this is to throw in a compat wrapper in
> glib-compat.h:
>
> --8<---------------cut here---------------start------------->8---
> modified include/glib-compat.h
> @@ -105,6 +105,24 @@ static inline gpointer g_memdup2_qemu(gconstpointer mem,
> gsize byte_size)
> }
> #define g_memdup2(m, s) g_memdup2_qemu(m, s)
>
> +/*
> + * g_pattern_match_string has been deprecated in Glib since 2.70 and
> + * will complain about it if you try to use it. Fortunately the
> + * signature of both functions is the same making it easy to work
> + * around.
> + */
> +static inline
> +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec,
> + const gchar *string)
> +{
> +#if GLIB_CHECK_VERSION(2, 70, 0)
> + return g_pattern_spec_match_string(pspec, string);
> +#else
> + return g_pattern_match_string(pspec, string);
> +#endif
> +};
> +#define g_pattern_spec_match_string(p, s)
> g_pattern_spec_match_string_qemu(p, s)
> +
> #if defined(G_OS_UNIX)
> /*
> * Note: The fallback implementation is not MT-safe, and it returns a copy of
> modified contrib/plugins/execlog.c
> @@ -334,8 +334,8 @@ static void 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 (g_pattern_match_string(pat, rd->name) ||
> - g_pattern_match_string(pat, rd_lower)) {
> + if (g_pattern_spec_match_string(pat, rd->name) ||
> + g_pattern_spec_match_string(pat, rd_lower)) {
> Register *reg = init_vcpu_register(vcpu_index, rd);
> g_ptr_array_add(registers, reg);
> --8<---------------cut here---------------end--------------->8---
>
> but I hesitated to add it for this case as plugins shouldn't assume they
> have access to QEMU's internals. Maybe the glib-compat.h header could be
> treated as a special case.
Thanks! This works on my side!
I support to fix the compatibility as the above, after all it's always
confusing when we allow users to use newer glib and see warnings at
compile time!
> >
> > I also noticed in target/arm/helper.c, there's another
> > g_pattern_match_string() but I haven't met the warning.
>
> Hmm that's weird. I suspect glib suppresses the warnings with:
>
> /* Ask for warnings for anything that was marked deprecated in
> * the defined version, or before. It is a candidate for rewrite.
> */
> #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56
>
I'm not too familiar with the QEMU build framework, but based on this, a
natural question is, can this rule be applied to plugins code as well?
If so, this would also avoid warning.
Thanks,
Zhao
