Zhao Liu <zhao1....@intel.com> writes: > 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?
If we g_strdup we have to remember to free later and ensure the g_ptr_array has a free func. > * Another way is the forced conversion to gpointer (also e.g., in > net/net.c, qemu_get_nic_models()). I think this is ok, but with a comment on all_reg_names just to remind any potential users that the strings are const and allocated by QEMU so are not to be modified or freed. > > Which way do you like? ;-) > <snip> >> >> 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. I think the simplest solution would be to add: #include "glib-compat.h" into qemu-plugin.h so plugins have the same deprecation rules as the QEMU they come from. I checked with Michael on IRC and Debian currently doesn't package any header files but if anyone starts shipping a qemu-dev package we'll need to make sure we include glib-compat.h in the same directory and qemu-plugin.h. > > Thanks, > Zhao -- Alex Bennée Virtualisation Tech Lead @ Linaro