Re: [PULL 26/29] contrib/plugins: extend execlog to track register changes

[Alex Bennée]

Zhao Liu <zhao1.liu@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.bennee@linaro.org>
>> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register
>>  changes
>> 
>> Zhao Liu <zhao1.liu@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