Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

["Alex Bennée" <alex.bennee@linaro.org>]

Nikos Filippakis <aesmade@gmail.com> writes:

> Hello everyone! I am interested in getting to know the codebase a little better
> so that I can eventually apply for a GSOC position.
> This is my first attempt at posting a patch to a mailing list, any feedback
> is greatly appreciated.

OK first things first this sort of meta comment belongs in the cover
letter. However for a single patch you may want to put such things below
the --- in the commit message as that will get stripped when the
maintainer eventually applies the patch. Otherwise your meta-comments
will end up in the version log ;-)

You'll see people use the --- area to keep version notes as patches go
through revisions.

>
> Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
> ---
>  net/net.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index aebf753..79e9d7c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
>  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
>                                 int iovcnt, unsigned flags)
>  {
> -    uint8_t buf[NET_BUFSIZE];
>      uint8_t *buffer;
>      size_t offset;
> +    ssize_t ret;

With that said your comment needs to explain why you've just made the
change. I see NET_BUFSIZE is quite large so maybe this should be a
clean-up across the rest of the code-base, what's so special about this
function? Have you measured any difference in performance?

>
>      if (iovcnt == 1) {
>          buffer = iov[0].iov_base;
>          offset = iov[0].iov_len;
>      } else {
> -        buffer = buf;
> -        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
> +        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
> +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
> +                            NET_BUFSIZE * sizeof(uint8_t));
>      }
>
>      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
> -        return nc->info->receive_raw(nc, buffer, offset);
> +        ret = nc->info->receive_raw(nc, buffer, offset);
>      } else {
> -        return nc->info->receive(nc, buffer, offset);
> +        ret = nc->info->receive(nc, buffer, offset);
>      }
> +
> +    if (iovcnt != 1) {
> +        g_free(buffer);
> +    }

This is a short function so you can get away with it but this sort of
logic can be confusing ("The iovec count was 1 therefor I should have
allocated a buffer" vs "I have an allocated buffer"). In general you
should know the various g_* functions tolerate NULLs well so maybe you
can structure the code differently (skipping the details ;-):

    uint8_t *buffer, *dynbuf = NULL;

    if (iovcnt == 1)
    {
      buffer = ...
    } else {
      buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
      ...
    }
    ...

    g_free(dynbuf)

> +
> +    return ret;
>  }
>
>  ssize_t qemu_deliver_packet_iov(NetClientState *sender,


--
Alex Bennée