| Message ID | 1449792930-27293-2-git-send-email-samuel.thibault@ens-lyon.org |
|---|---|
| State | New |
| Headers | show |
On 11/12/15 01:15, Samuel Thibault wrote: > From: Guillaume Subiron <maethor@subiron.org> > > Basically, this patch replaces "arp" by "resolution" every time "arp" > means "mac resolution" and not specifically ARP. > > Some indentation problems are solved in functions that will be modified > in the next patches (ip_input…). > > In if_encap, a switch is added to prepare for the IPv6 case. Some code > is factorized. > > Signed-off-by: Guillaume Subiron <maethor@subiron.org> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > slirp/mbuf.c | 2 +- > slirp/mbuf.h | 2 +- > slirp/slirp.c | 24 ++++++++++++++++++++---- > 3 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/slirp/mbuf.c b/slirp/mbuf.c > index 795fc29..bc942b6 100644 > --- a/slirp/mbuf.c > +++ b/slirp/mbuf.c > @@ -91,7 +91,7 @@ m_get(Slirp *slirp) > m->m_len = 0; > m->m_nextpkt = NULL; > m->m_prevpkt = NULL; > - m->arp_requested = false; > + m->resolution_requested = false; > m->expiration_date = (uint64_t)-1; > end_error: > DEBUG_ARG("m = %p", m); > diff --git a/slirp/mbuf.h b/slirp/mbuf.h > index b144f1c..38fedf4 100644 > --- a/slirp/mbuf.h > +++ b/slirp/mbuf.h > @@ -79,7 +79,7 @@ struct mbuf { > int m_len; /* Amount of data in this mbuf */ > > Slirp *slirp; > - bool arp_requested; > + bool resolution_requested; > uint64_t expiration_date; > /* start of dynamic buffer area, must be last element */ > union { > diff --git a/slirp/slirp.c b/slirp/slirp.c > index 35f819a..380ddc3 100644 > --- a/slirp/slirp.c > +++ b/slirp/slirp.c > @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) > return 1; > } > > + switch (iph->ip_v) { > + case IPVERSION: > if (iph->ip_dst.s_addr == 0) { > /* 0.0.0.0 can not be a destination address, something went wrong, > * avoid making it worse */ That indentation looks now broken - shouldn't the if-statement now be indented with four more spaces now? > @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) > struct ethhdr *reh = (struct ethhdr *)arp_req; > struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN); > > - if (!ifm->arp_requested) { > + if (!ifm->resolution_requested) { > /* If the client addr is not known, send an ARP request */ > memset(reh->h_dest, 0xff, ETH_ALEN); > memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4); > @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) > rah->ar_tip = iph->ip_dst.s_addr; > slirp->client_ipaddr = iph->ip_dst; > slirp_output(slirp->opaque, arp_req, sizeof(arp_req)); > - ifm->arp_requested = true; > + ifm->resolution_requested = true; > > /* Expire request and drop outgoing packet after 1 second */ > ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 1000000000ULL; > } > return 0; > } else { > - memcpy(eh->h_dest, ethaddr, ETH_ALEN); > memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4); > /* XXX: not correct */ > memcpy(&eh->h_source[2], &slirp->vhost_addr, 4); > eh->h_proto = htons(ETH_P_IP); > + break; > + } > + > + default: > + /* Do not assert while we don't manage IP6VERSION */ > + /* assert(0); */ > + break; > + } > + > + memcpy(eh->h_dest, ethaddr, ETH_ALEN); > + DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n", > + eh->h_source[0], eh->h_source[1], eh->h_source[2], > + eh->h_source[3], eh->h_source[4], eh->h_source[5])); > + DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n", > + eh->h_dest[0], eh->h_dest[1], eh->h_dest[2], > + eh->h_dest[3], eh->h_dest[4], eh->h_dest[5])); > memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len); > slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN); > return 1; > - } > } The lines after the switch statement should also only be indented with 4 spaces, not with 8. Also, I'd prefer if you could split this patch into two: One for renaming the "arp_requested" field, and one for adding the additional logic with the switch statement (since there are two different kind of changes). Thomas
On 11/12/15 14:38, Thomas Huth wrote: > On 11/12/15 01:15, Samuel Thibault wrote: >> From: Guillaume Subiron <maethor@subiron.org> >> >> Basically, this patch replaces "arp" by "resolution" every time "arp" >> means "mac resolution" and not specifically ARP. >> >> Some indentation problems are solved in functions that will be modified >> in the next patches (ip_input…). >> >> In if_encap, a switch is added to prepare for the IPv6 case. Some code >> is factorized. >> >> Signed-off-by: Guillaume Subiron <maethor@subiron.org> >> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> >> --- >> slirp/mbuf.c | 2 +- >> slirp/mbuf.h | 2 +- >> slirp/slirp.c | 24 ++++++++++++++++++++---- >> 3 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/slirp/mbuf.c b/slirp/mbuf.c >> index 795fc29..bc942b6 100644 >> --- a/slirp/mbuf.c >> +++ b/slirp/mbuf.c >> @@ -91,7 +91,7 @@ m_get(Slirp *slirp) >> m->m_len = 0; >> m->m_nextpkt = NULL; >> m->m_prevpkt = NULL; >> - m->arp_requested = false; >> + m->resolution_requested = false; >> m->expiration_date = (uint64_t)-1; >> end_error: >> DEBUG_ARG("m = %p", m); >> diff --git a/slirp/mbuf.h b/slirp/mbuf.h >> index b144f1c..38fedf4 100644 >> --- a/slirp/mbuf.h >> +++ b/slirp/mbuf.h >> @@ -79,7 +79,7 @@ struct mbuf { >> int m_len; /* Amount of data in this mbuf */ >> >> Slirp *slirp; >> - bool arp_requested; >> + bool resolution_requested; >> uint64_t expiration_date; >> /* start of dynamic buffer area, must be last element */ >> union { >> diff --git a/slirp/slirp.c b/slirp/slirp.c >> index 35f819a..380ddc3 100644 >> --- a/slirp/slirp.c >> +++ b/slirp/slirp.c >> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >> return 1; >> } >> >> + switch (iph->ip_v) { >> + case IPVERSION: >> if (iph->ip_dst.s_addr == 0) { >> /* 0.0.0.0 can not be a destination address, something went wrong, >> * avoid making it worse */ > > That indentation looks now broken - shouldn't the if-statement now be > indented with four more spaces now? > >> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >> struct ethhdr *reh = (struct ethhdr *)arp_req; >> struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN); >> >> - if (!ifm->arp_requested) { >> + if (!ifm->resolution_requested) { >> /* If the client addr is not known, send an ARP request */ >> memset(reh->h_dest, 0xff, ETH_ALEN); >> memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4); >> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >> rah->ar_tip = iph->ip_dst.s_addr; >> slirp->client_ipaddr = iph->ip_dst; >> slirp_output(slirp->opaque, arp_req, sizeof(arp_req)); >> - ifm->arp_requested = true; >> + ifm->resolution_requested = true; >> >> /* Expire request and drop outgoing packet after 1 second */ >> ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 1000000000ULL; >> } >> return 0; >> } else { >> - memcpy(eh->h_dest, ethaddr, ETH_ALEN); >> memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4); >> /* XXX: not correct */ >> memcpy(&eh->h_source[2], &slirp->vhost_addr, 4); >> eh->h_proto = htons(ETH_P_IP); >> + break; >> + } >> + >> + default: >> + /* Do not assert while we don't manage IP6VERSION */ >> + /* assert(0); */ >> + break; >> + } >> + >> + memcpy(eh->h_dest, ethaddr, ETH_ALEN); >> + DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n", >> + eh->h_source[0], eh->h_source[1], eh->h_source[2], >> + eh->h_source[3], eh->h_source[4], eh->h_source[5])); >> + DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n", >> + eh->h_dest[0], eh->h_dest[1], eh->h_dest[2], >> + eh->h_dest[3], eh->h_dest[4], eh->h_dest[5])); >> memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len); >> slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN); >> return 1; >> - } >> } > > The lines after the switch statement should also only be indented with 4 > spaces, not with 8. Ah, well, never mind, I did not see the comment in the patch description that you fix it up in the next patch. Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe it would be nicer to do it in one go instead (after splitting off the arp_requested renaming)? Thomas
Thomas Huth, on Fri 11 Dec 2015 14:38:44 +0100, wrote: > > + switch (iph->ip_v) { > > + case IPVERSION: > > if (iph->ip_dst.s_addr == 0) { > > /* 0.0.0.0 can not be a destination address, something went wrong, > > * avoid making it worse */ > > That indentation looks now broken - shouldn't the if-statement now be > indented with four more spaces now? Please see the patch summary: I was asked to provided a non-reindented patch and then a reindent patch. > Also, I'd prefer if you could split this patch into two: One for > renaming the "arp_requested" field, and one for adding the additional > logic with the switch statement (since there are two different kind of > changes). TBH I'm starting to again lose motivation. Samuel
Thomas Huth, on Fri 11 Dec 2015 14:43:42 +0100, wrote: > Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe > it would be nicer to do it in one go instead (after splitting off the > arp_requested renaming)? Please discuss about it with the people who asked for it. I won't revamp patches until what people prefer is settled. Personally I prefer the two-patch approach, since the first nicely shows what behavior change we have, and the second is a no-op patch, as can be seen with diff -w Samuel
On 11/12/15 14:43, Thomas Huth wrote: > On 11/12/15 14:38, Thomas Huth wrote: >> On 11/12/15 01:15, Samuel Thibault wrote: >>> From: Guillaume Subiron <maethor@subiron.org> >>> >>> Basically, this patch replaces "arp" by "resolution" every time "arp" >>> means "mac resolution" and not specifically ARP. >>> >>> Some indentation problems are solved in functions that will be modified >>> in the next patches (ip_input…). >>> >>> In if_encap, a switch is added to prepare for the IPv6 case. Some code >>> is factorized. >>> >>> Signed-off-by: Guillaume Subiron <maethor@subiron.org> >>> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> >>> --- >>> slirp/mbuf.c | 2 +- >>> slirp/mbuf.h | 2 +- >>> slirp/slirp.c | 24 ++++++++++++++++++++---- >>> 3 files changed, 22 insertions(+), 6 deletions(-) >>> >>> diff --git a/slirp/mbuf.c b/slirp/mbuf.c >>> index 795fc29..bc942b6 100644 >>> --- a/slirp/mbuf.c >>> +++ b/slirp/mbuf.c >>> @@ -91,7 +91,7 @@ m_get(Slirp *slirp) >>> m->m_len = 0; >>> m->m_nextpkt = NULL; >>> m->m_prevpkt = NULL; >>> - m->arp_requested = false; >>> + m->resolution_requested = false; >>> m->expiration_date = (uint64_t)-1; >>> end_error: >>> DEBUG_ARG("m = %p", m); >>> diff --git a/slirp/mbuf.h b/slirp/mbuf.h >>> index b144f1c..38fedf4 100644 >>> --- a/slirp/mbuf.h >>> +++ b/slirp/mbuf.h >>> @@ -79,7 +79,7 @@ struct mbuf { >>> int m_len; /* Amount of data in this mbuf */ >>> >>> Slirp *slirp; >>> - bool arp_requested; >>> + bool resolution_requested; >>> uint64_t expiration_date; >>> /* start of dynamic buffer area, must be last element */ >>> union { >>> diff --git a/slirp/slirp.c b/slirp/slirp.c >>> index 35f819a..380ddc3 100644 >>> --- a/slirp/slirp.c >>> +++ b/slirp/slirp.c >>> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> return 1; >>> } >>> >>> + switch (iph->ip_v) { >>> + case IPVERSION: >>> if (iph->ip_dst.s_addr == 0) { >>> /* 0.0.0.0 can not be a destination address, something went wrong, >>> * avoid making it worse */ >> >> That indentation looks now broken - shouldn't the if-statement now be >> indented with four more spaces now? >> >>> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> struct ethhdr *reh = (struct ethhdr *)arp_req; >>> struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN); >>> >>> - if (!ifm->arp_requested) { >>> + if (!ifm->resolution_requested) { >>> /* If the client addr is not known, send an ARP request */ >>> memset(reh->h_dest, 0xff, ETH_ALEN); >>> memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4); >>> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> rah->ar_tip = iph->ip_dst.s_addr; >>> slirp->client_ipaddr = iph->ip_dst; >>> slirp_output(slirp->opaque, arp_req, sizeof(arp_req)); >>> - ifm->arp_requested = true; >>> + ifm->resolution_requested = true; >>> >>> /* Expire request and drop outgoing packet after 1 second */ >>> ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 1000000000ULL; >>> } >>> return 0; >>> } else { >>> - memcpy(eh->h_dest, ethaddr, ETH_ALEN); >>> memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4); >>> /* XXX: not correct */ >>> memcpy(&eh->h_source[2], &slirp->vhost_addr, 4); >>> eh->h_proto = htons(ETH_P_IP); >>> + break; >>> + } >>> + >>> + default: >>> + /* Do not assert while we don't manage IP6VERSION */ >>> + /* assert(0); */ >>> + break; >>> + } >>> + >>> + memcpy(eh->h_dest, ethaddr, ETH_ALEN); >>> + DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n", >>> + eh->h_source[0], eh->h_source[1], eh->h_source[2], >>> + eh->h_source[3], eh->h_source[4], eh->h_source[5])); >>> + DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n", >>> + eh->h_dest[0], eh->h_dest[1], eh->h_dest[2], >>> + eh->h_dest[3], eh->h_dest[4], eh->h_dest[5])); >>> memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len); >>> slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN); >>> return 1; >>> - } >>> } >> >> The lines after the switch statement should also only be indented with 4 >> spaces, not with 8. > > Ah, well, never mind, I did not see the comment in the patch description > that you fix it up in the next patch. > > Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe > it would be nicer to do it in one go instead (after splitting off the > arp_requested renaming)? Or maybe even move the IPv4-only code into a separate function instead? ... otherwise if_encap() will be very huge once the IPv6 code has been added, I think. Thomas
On 11/12/15 14:47, Samuel Thibault wrote: > Thomas Huth, on Fri 11 Dec 2015 14:43:42 +0100, wrote: >> Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe >> it would be nicer to do it in one go instead (after splitting off the >> arp_requested renaming)? > > Please discuss about it with the people who asked for it. I won't > revamp patches until what people prefer is settled. It's maybe best if Jan Kiszka could comment on such question since he's the slirp subsystem maintainer and finally has to decide whether to take the patches or not. Thomas
Just to explain. I'm fine with revamping the patches, provided that we eventually converge somewhere. What I'm afraid of is that splitting patches in yet more many pieces, revamping the code yet more (moving code into their own functions, etc.) will only make the patch series look even bigger and even less prone to be included for lack of reviewer time. I agree that the presentation can be looked after. But if that makes the patch series much bigger, I'd really prefer to look after that once the content gets commited, otherwise I don't see how we'll ever manage to get this commited in the coming decade. Samuel
On 11/12/15 15:01, Samuel Thibault wrote: > Just to explain. > > I'm fine with revamping the patches, provided that we eventually > converge somewhere. > > What I'm afraid of is that splitting patches in yet more many pieces, > revamping the code yet more (moving code into their own functions, etc.) > will only make the patch series look even bigger and even less prone to > be included for lack of reviewer time. > > I agree that the presentation can be looked after. But if that makes the > patch series much bigger, I'd really prefer to look after that once the > content gets commited, otherwise I don't see how we'll ever manage to > get this commited in the coming decade. Ok, point taken, and I understand your frustration about reworking and posting this a couple of times without getting the patches accepted. So maybe it's better to do smaller steps instead: Would it for example make sense to split the whole series into two parts - first a series that does all the preparation and cleanup patches. And then once that has been reviewed and merged, send the second series that adds the real new IPv6 code. In the end, it's up to the subsystem mainter how the patches should be done - so, Jan, what do you think? What way would you recommend to get the patches ready for being included? Thomas
Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote: > So maybe it's better to do smaller steps instead: Would it for example > make sense to split the whole series into two parts - first a series > that does all the preparation and cleanup patches. And then once that > has been reviewed and merged, send the second series that adds the real > new IPv6 code. Ok, that's what we already have: patches 1-9 are refactoring and support, and 10-18 are ipv6 code. Samuel
On 11/12/15 15:55, Samuel Thibault wrote: > Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote: >> So maybe it's better to do smaller steps instead: Would it for example >> make sense to split the whole series into two parts - first a series >> that does all the preparation and cleanup patches. And then once that >> has been reviewed and merged, send the second series that adds the real >> new IPv6 code. > > Ok, that's what we already have: patches 1-9 are refactoring and > support, and 10-18 are ipv6 code. Sounds good, ... then I'd suggest to sent the preparation patches separately next time and get them accepted first. Thomas
meta On 12/11/15 16:09, Thomas Huth wrote: > On 11/12/15 15:55, Samuel Thibault wrote: >> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote: >>> So maybe it's better to do smaller steps instead: Would it for example >>> make sense to split the whole series into two parts - first a series >>> that does all the preparation and cleanup patches. And then once that >>> has been reviewed and merged, send the second series that adds the real >>> new IPv6 code. >> >> Ok, that's what we already have: patches 1-9 are refactoring and >> support, and 10-18 are ipv6 code. > > Sounds good, ... then I'd suggest to sent the preparation patches > separately next time and get them accepted first. And then the next reviewer will say, "nice, but it would be even nicer to see what *motivates* these preparatory patches!" :) Disclaimer: I don't have any technical context for this thread; I just noticed Samuel's email / frustration. I know that all too well, first hand, from this list and elsewhere. I don't know how it can be fixed, but I'm positive it is a *systemic* problem with this development model. (I didn't contribute much value with this email, but perhaps Samuel will feel better by seeing some (however unsolicited) confirmation; plus hey it's Friday.) Thanks Laszlo
Laszlo Ersek, on Fri 11 Dec 2015 16:40:32 +0100, wrote: > > Sounds good, ... then I'd suggest to sent the preparation patches > > separately next time and get them accepted first. > > And then the next reviewer will say, "nice, but it would be even nicer > to see what *motivates* these preparatory patches!" :) Yes, I also had that in mind, just didn't say it :) Samuel
On 12/11/2015 08:40 AM, Laszlo Ersek wrote: > meta > > On 12/11/15 16:09, Thomas Huth wrote: >> On 11/12/15 15:55, Samuel Thibault wrote: >>> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote: >>>> So maybe it's better to do smaller steps instead: Would it for example >>>> make sense to split the whole series into two parts - first a series >>>> that does all the preparation and cleanup patches. And then once that >>>> has been reviewed and merged, send the second series that adds the real >>>> new IPv6 code. >>> >>> Ok, that's what we already have: patches 1-9 are refactoring and >>> support, and 10-18 are ipv6 code. >> >> Sounds good, ... then I'd suggest to sent the preparation patches >> separately next time and get them accepted first. > > And then the next reviewer will say, "nice, but it would be even nicer > to see what *motivates* these preparatory patches!" :) > > Disclaimer: I don't have any technical context for this thread; I just > noticed Samuel's email / frustration. I know that all too well, first > hand, from this list and elsewhere. I don't know how it can be fixed, > but I'm positive it is a *systemic* problem with this development model. The best defense you have against this is to put more information in a commit message - any time you have split work to ease review, make sure to mention it in the commit message. Any time you choose to merge work into a single patch for less churn, mention it. The commit message is your greatest chance of explaining to reviewers WHY you did it the way you did; and a reasonable reviewer should be happy enough that you did the work without making you redo it to match their 'ivory tower' alternative approach that meets the same end. If you changed a patch from an earlier version due to a particular reviewer, feel free to mention that reviewer in your explanation of changes (although in this case, doing it in the cover letter or after the --- marker may be better than in the commit body proper). Yes, I've also been on the receiving end of frustration of my patch not being perfect enough, and try to be relatively forgiving of different styles when they aren't outright forbidden by the code base's written standards (there's a huge difference between a patch not being technically correct, and merely not being stylistically ideal according to my ideals). Also, projects that automate standards checks (such as our scripts/checkpatch.pl) are nicer than ones that require contributors to comply with unwritten rules - but even then you can only encode so much into an automated checker, and still need to leave room for human judgment on when to bend the rules. At any rate, if you ever feel like you are being asked to do too much churn for no real benefit, please call attention to that fact. Burn out is real, and suffering in silence is not beneficial to the project.
On 12/11/15 17:17, Eric Blake wrote: > On 12/11/2015 08:40 AM, Laszlo Ersek wrote: >> meta >> >> On 12/11/15 16:09, Thomas Huth wrote: >>> On 11/12/15 15:55, Samuel Thibault wrote: >>>> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote: >>>>> So maybe it's better to do smaller steps instead: Would it for example >>>>> make sense to split the whole series into two parts - first a series >>>>> that does all the preparation and cleanup patches. And then once that >>>>> has been reviewed and merged, send the second series that adds the real >>>>> new IPv6 code. >>>> >>>> Ok, that's what we already have: patches 1-9 are refactoring and >>>> support, and 10-18 are ipv6 code. >>> >>> Sounds good, ... then I'd suggest to sent the preparation patches >>> separately next time and get them accepted first. >> >> And then the next reviewer will say, "nice, but it would be even nicer >> to see what *motivates* these preparatory patches!" :) >> >> Disclaimer: I don't have any technical context for this thread; I just >> noticed Samuel's email / frustration. I know that all too well, first >> hand, from this list and elsewhere. I don't know how it can be fixed, >> but I'm positive it is a *systemic* problem with this development model. > > The best defense you have against this is to put more information in a > commit message - any time you have split work to ease review, make sure > to mention it in the commit message. Any time you choose to merge work > into a single patch for less churn, mention it. The commit message is > your greatest chance of explaining to reviewers WHY you did it the way > you did; and a reasonable reviewer should be happy enough that you did > the work without making you redo it to match their 'ivory tower' > alternative approach that meets the same end. If you changed a patch > from an earlier version due to a particular reviewer, feel free to > mention that reviewer in your explanation of changes (although in this > case, doing it in the cover letter or after the --- marker may be better > than in the commit body proper). > > Yes, I've also been on the receiving end of frustration of my patch not > being perfect enough, and try to be relatively forgiving of different > styles when they aren't outright forbidden by the code base's written > standards (there's a huge difference between a patch not being > technically correct, and merely not being stylistically ideal according > to my ideals). I'm very impressed that you are conscious of this. I also seek to remind myself of it (in the reviewer role), frequently. Thank you. > Also, projects that automate standards checks (such as > our scripts/checkpatch.pl) are nicer than ones that require contributors > to comply with unwritten rules - but even then you can only encode so > much into an automated checker, and still need to leave room for human > judgment on when to bend the rules. > > At any rate, if you ever feel like you are being asked to do too much > churn for no real benefit, please call attention to that fact. Burn out > is real, and suffering in silence is not beneficial to the project. I used to complain very loudly ("suffering in silence" is not what I'm naturally inclined to do :)), but recently I've had zero problems on qemu-devel, luckily. My only reason to go meta here was Samuel's emails, which looked all too familiar to my own earlier ones. (This is not to say that I take issue with whatever Thomas or other reviewers may have said -- I have no clue what they said; I just noticed the "pattern" in Samuel's emails.) Thanks! Laszlo
Thomas Huth, on Fri 11 Dec 2015 14:38:44 +0100, wrote: > Also, I'd prefer if you could split this patch into two: One for > renaming the "arp_requested" field, and one for adding the additional > logic with the switch statement (since there are two different kind of > changes). Ok, at least it's easy enough :) Samuel
diff --git a/slirp/mbuf.c b/slirp/mbuf.c index 795fc29..bc942b6 100644 --- a/slirp/mbuf.c +++ b/slirp/mbuf.c @@ -91,7 +91,7 @@ m_get(Slirp *slirp) m->m_len = 0; m->m_nextpkt = NULL; m->m_prevpkt = NULL; - m->arp_requested = false; + m->resolution_requested = false; m->expiration_date = (uint64_t)-1; end_error: DEBUG_ARG("m = %p", m); diff --git a/slirp/mbuf.h b/slirp/mbuf.h index b144f1c..38fedf4 100644 --- a/slirp/mbuf.h +++ b/slirp/mbuf.h @@ -79,7 +79,7 @@ struct mbuf { int m_len; /* Amount of data in this mbuf */ Slirp *slirp; - bool arp_requested; + bool resolution_requested; uint64_t expiration_date; /* start of dynamic buffer area, must be last element */ union { diff --git a/slirp/slirp.c b/slirp/slirp.c index 35f819a..380ddc3 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) return 1; } + switch (iph->ip_v) { + case IPVERSION: if (iph->ip_dst.s_addr == 0) { /* 0.0.0.0 can not be a destination address, something went wrong, * avoid making it worse */ @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) struct ethhdr *reh = (struct ethhdr *)arp_req; struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN); - if (!ifm->arp_requested) { + if (!ifm->resolution_requested) { /* If the client addr is not known, send an ARP request */ memset(reh->h_dest, 0xff, ETH_ALEN); memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4); @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) rah->ar_tip = iph->ip_dst.s_addr; slirp->client_ipaddr = iph->ip_dst; slirp_output(slirp->opaque, arp_req, sizeof(arp_req)); - ifm->arp_requested = true; + ifm->resolution_requested = true; /* Expire request and drop outgoing packet after 1 second */ ifm->expiration_date = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + 1000000000ULL; } return 0; } else { - memcpy(eh->h_dest, ethaddr, ETH_ALEN); memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4); /* XXX: not correct */ memcpy(&eh->h_source[2], &slirp->vhost_addr, 4); eh->h_proto = htons(ETH_P_IP); + break; + } + + default: + /* Do not assert while we don't manage IP6VERSION */ + /* assert(0); */ + break; + } + + memcpy(eh->h_dest, ethaddr, ETH_ALEN); + DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n", + eh->h_source[0], eh->h_source[1], eh->h_source[2], + eh->h_source[3], eh->h_source[4], eh->h_source[5])); + DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n", + eh->h_dest[0], eh->h_dest[1], eh->h_dest[2], + eh->h_dest[3], eh->h_dest[4], eh->h_dest[5])); memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len); slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN); return 1; - } } /* Drop host forwarding rule, return 0 if found. */