[Qemu-devel] [PATCH 0/5] Small optimizations for code using g_malloc0 + memset/memcpy

[Qemu-devel] [PATCH 0/5] Small optimizations for code using g_malloc0 + memset/memcpy

[Thomas Huth]

There are a couple of spots in the QEMU code which use g_malloc0,
directly followed by a memset or memcpy which fill the whole
allocated buffer. In this case it either does not make sense to
zero the buffer via g_malloc0 first (so g_malloc should be used
instead), or if the second command is a memset(..., 0, ...), then
the memset does not make much sense, of course, since the buffer
has already been zeroed by the g_malloc0.

Thomas Huth (5):
  hw/dma/pxa2xx: Remove superfluous memset
  hw/scsi/spapr_vscsi: Remove superfluous memset
  hw/input/tsc210x: Remove superfluous memset
  tests/i44fx-test: No need for zeroing memory before memset
  linux-user/syscall: Replace g_malloc0 + memcpy with g_memdup

 hw/dma/pxa2xx_dma.c   | 1 -
 hw/input/tsc210x.c    | 8 ++------
 hw/scsi/spapr_vscsi.c | 1 -
 linux-user/syscall.c  | 3 +--
 tests/i440fx-test.c   | 2 +-
 5 files changed, 4 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] hw/dma/pxa2xx: Remove superfluous memset

[Thomas Huth]

g_malloc0 already clears the memory, so no need for
the additional memset here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/dma/pxa2xx_dma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/dma/pxa2xx_dma.c b/hw/dma/pxa2xx_dma.c
index d4501fb..3f0a720 100644
--- a/hw/dma/pxa2xx_dma.c
+++ b/hw/dma/pxa2xx_dma.c
@@ -461,7 +461,6 @@ static int pxa2xx_dma_init(SysBusDevice *sbd)
 
     s->chan = g_malloc0(sizeof(PXA2xxDMAChannel) * s->channels);
 
-    memset(s->chan, 0, sizeof(PXA2xxDMAChannel) * s->channels);
     for (i = 0; i < s->channels; i ++)
         s->chan[i].state = DCSR_STOPINTR;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/5] hw/scsi/spapr_vscsi: Remove superfluous memset

[Thomas Huth]

g_malloc0 already clears the memory, so no need for
the additional memset here.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/scsi/spapr_vscsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 891424f..f4f5140 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -750,7 +750,6 @@ static void vscsi_report_luns(VSCSIState *s, vscsi_req *req)
     len = n+8;
 
     resp_data = g_malloc0(len);
-    memset(resp_data, 0, len);
     stl_be_p(resp_data, n);
     i = found_lun0 ? 8 : 16;
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] hw/input/tsc210x: Remove superfluous memset

[Thomas Huth]

g_malloc0 already clears the memory, so no need for additional
memsets here. And while we're at it, let's also remove the
superfluous typecasts for the return values of g_malloc0.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/input/tsc210x.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index fae3385..92b076a 100644
--- a/hw/input/tsc210x.c
+++ b/hw/input/tsc210x.c
@@ -1086,9 +1086,7 @@ uWireSlave *tsc2102_init(qemu_irq pint)
 {
     TSC210xState *s;
 
-    s = (TSC210xState *)
-            g_malloc0(sizeof(TSC210xState));
-    memset(s, 0, sizeof(TSC210xState));
+    s = g_malloc0(sizeof(TSC210xState));
     s->x = 160;
     s->y = 160;
     s->pressure = 0;
@@ -1135,9 +1133,7 @@ uWireSlave *tsc2301_init(qemu_irq penirq, qemu_irq kbirq, qemu_irq dav)
 {
     TSC210xState *s;
 
-    s = (TSC210xState *)
-            g_malloc0(sizeof(TSC210xState));
-    memset(s, 0, sizeof(TSC210xState));
+    s = g_malloc0(sizeof(TSC210xState));
     s->x = 400;
     s->y = 240;
     s->pressure = 0;
-- 
1.8.3.1

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

[Thomas Huth]

Change a g_malloc0 into g_malloc since the following
memset fills the whole buffer anyway.

Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/5] linux-user/syscall: Replace g_malloc0 + memcpy with g_memdup

[Thomas Huth]

No need to use g_malloc0 to zero the memory if we memcpy to
the whole buffer afterwards anyway. Actually, there is even
a function which combines both steps, g_memdup, so let's use
this function here instead.

Cc: Riku Voipio <riku.voipio@iki.fi>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 linux-user/syscall.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 98b5766..f2ada0a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5325,8 +5325,7 @@ static abi_long do_open_by_handle_at(abi_long mount_fd, abi_long handle,
         return -TARGET_EFAULT;
     }
 
-    fh = g_malloc0(total_size);
-    memcpy(fh, target_fh, total_size);
+    fh = g_memdup(target_fh, total_size);
     fh->handle_bytes = size;
     fh->handle_type = tswap32(target_fh->handle_type);
 
-- 
1.8.3.1

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

[Laszlo Ersek]

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 <lersek@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  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?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 1/5] hw/dma/pxa2xx: Remove superfluous memset

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On 10/08/2015 01:35 PM, Thomas Huth wrote:
> g_malloc0 already clears the memory, so no need for
> the additional memset here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/dma/pxa2xx_dma.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/dma/pxa2xx_dma.c b/hw/dma/pxa2xx_dma.c
> index d4501fb..3f0a720 100644
> --- a/hw/dma/pxa2xx_dma.c
> +++ b/hw/dma/pxa2xx_dma.c
> @@ -461,7 +461,6 @@ static int pxa2xx_dma_init(SysBusDevice *sbd)
>  
>      s->chan = g_malloc0(sizeof(PXA2xxDMAChannel) * s->channels);

As long as we're here, this should probably be switched to g_new0(), in
part to avoid any chance that the multiply can overflow.

>  
> -    memset(s->chan, 0, sizeof(PXA2xxDMAChannel) * s->channels);

But this part is correct.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/5] hw/scsi/spapr_vscsi: Remove superfluous memset

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On 10/08/2015 01:35 PM, Thomas Huth wrote:
> g_malloc0 already clears the memory, so no need for
> the additional memset here.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/scsi/spapr_vscsi.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] linux-user/syscall: Replace g_malloc0 + memcpy with g_memdup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On 10/08/2015 01:35 PM, Thomas Huth wrote:
> No need to use g_malloc0 to zero the memory if we memcpy to
> the whole buffer afterwards anyway. Actually, there is even
> a function which combines both steps, g_memdup, so let's use
> this function here instead.
> 
> Cc: Riku Voipio <riku.voipio@iki.fi>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  linux-user/syscall.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 98b5766..f2ada0a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5325,8 +5325,7 @@ static abi_long do_open_by_handle_at(abi_long mount_fd, abi_long handle,
>          return -TARGET_EFAULT;
>      }
>  
> -    fh = g_malloc0(total_size);
> -    memcpy(fh, target_fh, total_size);
> +    fh = g_memdup(target_fh, total_size);
>      fh->handle_bytes = size;
>      fh->handle_type = tswap32(target_fh->handle_type);
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] hw/input/tsc210x: Remove superfluous memset

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

On 10/08/2015 01:35 PM, Thomas Huth wrote:
> g_malloc0 already clears the memory, so no need for additional
> memsets here. And while we're at it, let's also remove the
> superfluous typecasts for the return values of g_malloc0.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/input/tsc210x.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
> index fae3385..92b076a 100644
> --- a/hw/input/tsc210x.c
> +++ b/hw/input/tsc210x.c
> @@ -1086,9 +1086,7 @@ uWireSlave *tsc2102_init(qemu_irq pint)
>  {
>      TSC210xState *s;
>  
> -    s = (TSC210xState *)
> -            g_malloc0(sizeof(TSC210xState));
> -    memset(s, 0, sizeof(TSC210xState));
> +    s = g_malloc0(sizeof(TSC210xState));

This should probably be g_new0(TSC210xState, 1), consistent with Markus'
recent cleanup patches.

>      s->x = 160;
>      s->y = 160;
>      s->pressure = 0;
> @@ -1135,9 +1133,7 @@ uWireSlave *tsc2301_init(qemu_irq penirq, qemu_irq kbirq, qemu_irq dav)
>  {
>      TSC210xState *s;
>  
> -    s = (TSC210xState *)
> -            g_malloc0(sizeof(TSC210xState));
> -    memset(s, 0, sizeof(TSC210xState));
> +    s = g_malloc0(sizeof(TSC210xState));

Likewise.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/5] hw/scsi/spapr_vscsi: Remove superfluous memset

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Thu, Oct 08, 2015 at 09:35:13PM +0200, Thomas Huth wrote:
> g_malloc0 already clears the memory, so no need for
> the additional memset here.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks, applied to spapr-next.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

On 08/10/15 22:24, Laszlo Ersek wrote:
> 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 <lersek@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  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?

g_new IMHO only makes sense when you try to allocate the memory for a
struct or something similar - for allocating byte arrays, g_malloc is
the better choice. So I think this patch should be fine.

 Thomas

Re: [Qemu-devel] [PATCH 1/5] hw/dma/pxa2xx: Remove superfluous memset

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 881 bytes --]

On 08/10/15 22:59, Eric Blake wrote:
> On 10/08/2015 01:35 PM, Thomas Huth wrote:
>> g_malloc0 already clears the memory, so no need for
>> the additional memset here.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/dma/pxa2xx_dma.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/dma/pxa2xx_dma.c b/hw/dma/pxa2xx_dma.c
>> index d4501fb..3f0a720 100644
>> --- a/hw/dma/pxa2xx_dma.c
>> +++ b/hw/dma/pxa2xx_dma.c
>> @@ -461,7 +461,6 @@ static int pxa2xx_dma_init(SysBusDevice *sbd)
>>  
>>      s->chan = g_malloc0(sizeof(PXA2xxDMAChannel) * s->channels);
> 
> As long as we're here, this should probably be switched to g_new0(), in
> part to avoid any chance that the multiply can overflow.

Ok, I doubt that we really get overflows here, but g_new0 is certainly
nicer here, so I'll change my patch accordingly.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/5] hw/input/tsc210x: Remove superfluous memset

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 09/10/15 01:34, Eric Blake wrote:
> On 10/08/2015 01:35 PM, Thomas Huth wrote:
>> g_malloc0 already clears the memory, so no need for additional
>> memsets here. And while we're at it, let's also remove the
>> superfluous typecasts for the return values of g_malloc0.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/input/tsc210x.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
>> index fae3385..92b076a 100644
>> --- a/hw/input/tsc210x.c
>> +++ b/hw/input/tsc210x.c
>> @@ -1086,9 +1086,7 @@ uWireSlave *tsc2102_init(qemu_irq pint)
>>  {
>>      TSC210xState *s;
>>  
>> -    s = (TSC210xState *)
>> -            g_malloc0(sizeof(TSC210xState));
>> -    memset(s, 0, sizeof(TSC210xState));
>> +    s = g_malloc0(sizeof(TSC210xState));
> 
> This should probably be g_new0(TSC210xState, 1), consistent with Markus'
> recent cleanup patches.

Ok, I'll change my patch.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  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.

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

[Laszlo Ersek]

On 10/09/15 08:46, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  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.
> 

Okay, thanks!

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Cheers
Laszlo