From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33674) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acppX-0006fb-HB for qemu-devel@nongnu.org; Mon, 07 Mar 2016 02:43:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acppS-0002pK-Fq for qemu-devel@nongnu.org; Mon, 07 Mar 2016 02:43:51 -0500 Received: from mail-wm0-x232.google.com ([2a00:1450:400c:c09::232]:36653) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acppS-0002pF-5V for qemu-devel@nongnu.org; Mon, 07 Mar 2016 02:43:46 -0500 Received: by mail-wm0-x232.google.com with SMTP id n186so72568215wmn.1 for ; Sun, 06 Mar 2016 23:43:45 -0800 (PST) References: <1457210381-23325-1-git-send-email-aesmade@gmail.com> <871t7ogia3.fsf@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Mon, 07 Mar 2016 07:43:43 +0000 Message-ID: <877fhe3f80.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: > Thank you for your comments! > > On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée wrote: >> >> >> 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. >> > > I thought that could be considered part of the cover letter, didn't > realize it would end up on the version log. Sorry about that (: When you use git format-patch with --cover-letter to format a series of patches you'll get exactly that. For individual patches like this one then bellow the --- works. The fact your a potential GSOC student is useful information to us on the list, just not in the actual commit log in git ;-) > >> > >> > 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? >> > > This method is one of several mentioned on the wiki as having big > stack frames because of such arrays, something > someone new to the project could easily fix, either by moving it to > the heap or reducing the array size. Since further > down there is a call to memcpy with NET_BUFSIZE length, I thought I'd > just move it to the heap. That's fine. In fact referencing the wiki bite-sized tasks would probably be enough context for the commit message. > Nothing special about this method, I'm planning to do the same with > the others, just trying to get some > familiarity with the mailing list. Don't worry too much, it usually takes a few attempts to get your first patch applied and the workflow sorted out. > Besides, I'm not sure if I should put such small changes to different > files in many small commits, or a large one. The key byword here is bisectability. If regressions get introduced we want to be able to quickly identify the culprit with git-bisect. So it is important that every commit in the project builds cleanly. For something like this I'd argue a series of patches would make sense as they are likely in different functional places in the code. > >> > >> > 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) >> > > You're right, I didn't quite like the way I did it either. I'm > resubmitting it, hopefully fixing these mistakes. This is more a question of style rather than mistakes in the code. However taste is a good guide, while sometimes code is as ugly as it needs to be it is often worthwhile investigating alternatives if your initial reaction is ambivalent. > >> > + >> > + return ret; >> > } >> > >> > ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> >> >> -- >> Alex Bennée -- Alex Bennée