Nikos Filippakis <aesm...@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 <aesm...@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
