Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Blue Swirl]

On Tue, Oct 18, 2011 at 1:13 AM, Stuart Brady <address@hidden> wrote:
> On Mon, Oct 17, 2011 at 10:01:25PM +0200, Stefan Weil wrote:
>>
>> The patch also slightly cleans the g_malloc0 statement which was
>> touched by that change (no type cast, easier code review).
> [...]
>> -    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
> [...]
>> +    s = g_malloc0(sizeof(*s));
>
> Is this the preferred style, or should we be using:
>
>    s = g_new0(GDBRegisterState, 1);
>
> We currently seem to have a mix of the two styles.
>
> I kinda prefer using g_new() (or g_new0()) since provided the 'count'
> parameter is correct, it can't really be wrong, whereas using sizeof()
> has the potential to be buggy, even if it is still trivial to check
> that the code is okay.
>
> I guess I feel g_malloc() would be best reserved for those cases where
> we're doing something special which might need a little extra care...
>
> BTW, I've just been experimenting with Coccinelle and produced the
> following semantic patch:
>
>   @@ type T; @@
>   -(T *)g_malloc(sizeof(T))
>   +g_new(T, 1)
>
>   @@ type T; @@
>   -(T *)g_malloc0(sizeof(T))
>   +g_new0(T, 1)
>
>   @@ type T; expression E; @@
>   -(T *)g_malloc(sizeof(T) * E)
>   +g_new(T, E)
>
>   @@ type T; expression E; @@
>   -(T *)g_malloc0(sizeof(T) * E)
>   +g_new0(T, E)

Cool. Please include the spatch with the commit message.

> I couldn't get the following working:
>
>   // THIS DOES NOT WORK:
>   @@ type T; identifier I; @@
>   -T I = g_malloc(sizeof(*I))
>   +T I = g_new(T, 1)
>
> I expect this won't work either:
>
>   // THIS PROBABLY DOES NOT WORK EITHER:
>   @@ type T; identifier I; @@
>   -T I = g_new(T, 1)
>   +T I = g_malloc(sizeof(*I))

IIRC I had also problems with identifiers, I could not get checking
identifiers against CODING_STYLE to work.