From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acTPA-0002tp-ND for qemu-devel@nongnu.org; Sun, 06 Mar 2016 02:47:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acTP6-0000CO-Md for qemu-devel@nongnu.org; Sun, 06 Mar 2016 02:47:08 -0500 Received: from mail-wm0-x22d.google.com ([2a00:1450:400c:c09::22d]:38128) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acTP6-0000C5-BG for qemu-devel@nongnu.org; Sun, 06 Mar 2016 02:47:04 -0500 Received: by mail-wm0-x22d.google.com with SMTP id l68so36787633wml.1 for ; Sat, 05 Mar 2016 23:47:03 -0800 (PST) References: <1457210381-23325-1-git-send-email-aesmade@gmail.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1457210381-23325-1-git-send-email-aesmade@gmail.com> Date: Sun, 06 Mar 2016 07:47:00 +0000 Message-ID: <871t7ogia3.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikos Filippakis Cc: jasowang@redhat.com, qemu-devel@nongnu.org Nikos Filippakis 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 > --- > 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