Re: [Qemu-devel] [PATCH 5/5] coccinelle: prefer glib g_new/g_renew macros

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> ----- Original Message -----
>> On 06/07/2017 02:46 AM, Marc-André Lureau wrote:
>> > The g_new() familly of macros is simpler and safer than g_malloc().
>> 
>> s/familly/family/
>> 
>> > 
>> > "The return pointer is cast to the given type... Care is taken to
>> > avoid overflow when calculating the size of the allocated block."
>> > 
>> > I left out the common g_malloc(sizeof(*ptr)) pattern, since
>> > alternative "g_new(typeof(*ptr))" isn't very common. But we may want
>> > to change that too?
>> 
>> Markus has made changes like this in the past (see commits bdd81add,
>> b45c03f, ...).  It may even be worth cribbing his commit messages,
>> and/or converting his Coccinelle script into something stored in the
>> repository, since we tend to re-run it and find more poor uses that have
>> crept in over time.
>
> I don't think it's so simple to write a full and correct script that is worth 
> being stored in tree. At least, I don't have enough experience to do so.
>  
>> > 
>> > Here is the cocci script I used, then I edited manually a few
>> > changes (I removed useless cast for ex):
>> > 
>> > @@
>> > expression e1;
>> > expression e2;
>> > expression mem;
>> > type t1;
>> > @@
>> 
>> Your script differs from Markus', we should figure out if they can be
>> merged into one.
>
> One notable difference is that I abuse expression, instead of type. I didn't 
> manage to teach spatch about the includes and custom type (--all-includes 
> didn't work). I just tried with expression and it was happy, I haven't 
> searched further.

Does your semantic patch more, less, or both?

>> 
>> > (
>> > - g_malloc0(sizeof(*e2))
>> > + g_malloc0(sizeof(*e2))
>> 
>> Huh?
>> 
>> > |
>> > - g_malloc(sizeof(*e2))
>> > + g_malloc(sizeof(*e2))
>> 
>> Huh?
>
> That's what I explained in the cover letter, I don't wont those to be 
> touched, but they would because I abuse expressions...
>  
>> 
>> > |
>> > - g_realloc(mem, (e1) * sizeof(*e2))
>> > + g_renew(typeof(*e2), mem, e1)
>> 
>> We haven't used typeof() very frequently. I don't know if it is worth
>> using more frequently, maybe Markus has an opinion.
>> 
>> > |
>> > - g_malloc0((e1) * sizeof(*e2))
>> > + g_new0(typeof(*e2), e1)
>> > |
>> > - g_malloc((e1) * sizeof(*e2))
>> > + g_new(typeof(*e2), e1)
>> > |
>> > - g_realloc(mem, (e1) * sizeof(e2[0]))
>> > + g_renew(typeof(e2[0]), mem, e1)
>> 
>> Ditto.
>> 
>> > |
>> > - g_realloc(mem, (e1) * sizeof(e2))
>> > + g_renew(e2, mem, e1)
>> 
>> This one makes sense.
>> 
>> > Signed-off-by: Marc-André Lureau <address@hidden>
[...]
>> >  161 files changed, 278 insertions(+), 285 deletions(-)
>> 
>> That's big; I'd rather we get consensus on the Coccinelle script first,
>> and then review the fallout.  Last time I did a .cocci patch that was
>> worth having in the tree, I specifically split the addition of the
>> script from running the script, to make backporting slightly easier
>> (backport the script as-is, then re-run the formula in the commit
>> message of the application, which is easier than hand-verifying conflict
>> resolutions over time).
>
> Sadly, my script is really far from perfect. And I don't how much time it 
> will take me to make it better, and if I really want to spend that time for 
> this. In any case, the result needs careful review. So thought it would be 
> easier to provide a patch that I manually changed/reviewed, rather than a 
> full cocci script.

I can play with the script when this series reaches the front of my
review queue.

Manual review might be easier if you split the patch along patterns.

To get similar patches merged in the past, I had to split along split
along subsystems as well.  The typical split is major subsystems with
active maintainers, plus a part of miscellaneous leftovers.

>
>> > 
>> > diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
>> > index a01f6bc5df..38ade3db0e 100644
>> > --- a/hw/lm32/lm32_hwsetup.h
>> > +++ b/hw/lm32/lm32_hwsetup.h
>> > @@ -58,7 +58,7 @@ static inline HWSetup *hwsetup_init(void)
>> >  {
>> >      HWSetup *hw;
>> >  
>> > -    hw = g_malloc(sizeof(HWSetup));
>> > +    hw = g_new(HWSetup, 1);
>> 
>> At any rate, cleanups like this match what we have done in the past, so
>> you're on the right track, even though I'm not giving R-b yet.