Re: [Qemu-trivial] [Qemu-devel] [PATCH 4/5] tests/i44fx-test: No need for zeroing memory before memset

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 10/08/15 21:35, Thomas Huth wrote:
>> Change a g_malloc0 into g_malloc since the following
>> memset fills the whole buffer anyway.
>> 
>> Cc: Laszlo Ersek <address@hidden>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  tests/i440fx-test.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
>> index d0bc8de..7fa1709 100644
>> --- a/tests/i440fx-test.c
>> +++ b/tests/i440fx-test.c
>> @@ -191,7 +191,7 @@ static void write_area(uint32_t start, uint32_t end, 
>> uint8_t value)
>>      uint32_t size = end - start + 1;
>>      uint8_t *data;
>>  
>> -    data = g_malloc0(size);
>> +    data = g_malloc(size);
>>      memset(data, value, size);
>>      memwrite(start, data, size);
>>  
>> 
>
> Technically you are right of course, but I remember some historical mess
> around this, in this file.
>
> Plus I vaguely recall g_new[0]() being the most recent preference.
>
> https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-new
>
> See e.g. commit 97f3ad3551. Markus?

TL;DR: the patch is fine.

The argument of g_malloc() isn't always the size of a type.  But when it
is, you should be using g_new() instead.  The commit message explains:

    g_new(T, n) is neater than g_malloc(sizeof(T) * n).  It's also safer,
    for two reasons.  One, it catches multiplication overflowing size_t.
    Two, it returns T * rather than void *, which lets the compiler catch
    more type errors.
    
The multiplication argument applies only when n != 1.

The type checking argument applies regardless of n.  That's why the
commit also transfroms patterns like g_malloc(sizeof(T)).

When the argument isn't written as sizeof a type, but could be, we enter
"matter of taste" territory.  For instance, some may perefer the
idiomatic T *p = g_malloc(sizeof(*p)) to the more tightly typed
p = g_new(T, 1).  I therefore leave them alone.  Commit messsage again:

    This commit only touches allocations with size arguments of the form
    sizeof(T).

A separate issue is a zeroing memset() after allocation.  Please use a
zeroing allocator instead, because that's more concise.  Precedence:
commit 0bd0adb.

Now let's apply this to the patch.

The memset() isn't zeroing, so "use a zeroing allocator instead" doesn't
apply.  Before the patch, the code uses one additionally, which is
wasteful.  The patch stops the waste.

The size argument is not the size of any type.  You could still write

    data = g_new(uint8_t, size);

but the extra verbosity pretty clearly outweighs what we could gain from
type checking here.