Nikos Filippakis <aesm...@gmail.com> writes:
> Thank you for your comments!
>
> On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée <alex.ben...@linaro.org> wrote:
>>
>>
>> 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.
>>
>
> 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 <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?
>>
>
> 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
