Re: [Qemu-devel] [PATCH v2 1/1] socket: Allocating Large sized arrays to heap

[Markus Armbruster]

Pooja Dhannawat <address@hidden> writes:

> On Mon, Mar 14, 2016 at 10:20 PM, Daniel P. Berrange <address@hidden>
> wrote:
>
>> On Mon, Mar 14, 2016 at 10:07:53PM +0530, Pooja Dhannawat wrote:
>> > net_socket_send has a huge stack usage of 69712 bytes approx.
>> > Moving large arrays to heap to reduce stack usage.
>> >
>> > Signed-off-by: Pooja Dhannawat <address@hidden>
>> > ---
>> >  net/socket.c | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/net/socket.c b/net/socket.c
>> > index e32e3cb..3fcd7a6 100644
>> > --- a/net/socket.c
>> > +++ b/net/socket.c
>> > @@ -147,10 +147,10 @@ static void net_socket_send(void *opaque)
>> >      NetSocketState *s = opaque;
>> >      int size, err;
>> >      unsigned l;
>> > -    uint8_t buf1[NET_BUFSIZE];
>> > +    uint8_t *buf1 = g_new(uint8_t, NET_BUFSIZE);
>>
>> You're allocating NET_BUFSIZE worth of uint8_t's
>>
>> I didn't get you clear.
>
>
>> >      const uint8_t *buf;
>> >
>> > -    size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
>> > +    size = qemu_recv(s->fd, (uint8_t *)buf1, sizeof(uint8_t), 0);
>>
>> But only reading 1 byte which is clearly wrong. You likely wanted
>> NET_BUFSIZE here, not sizeof(uint8_t)
>>
>> Correct me If I am wrong. This should also work :
> size = qemu_recv(s->fd, (uint8_t *)buf1, NET_BUFSIZE * sizeof(uint8_t), 0);

An expression of type "T[]" deteriorates to "T *" without a cast.
Therefore, your cast of buf1 to uint8_t * is redundant and clutters the
code, please drop it.

In general, try hard to avoid casts not just to reduce clutter, but also
because casts can hide mistakes from the type checker.

sizeof(uint8_t) is always one.  Multiplying by it clutters the code,
please drop it.