On 11/12/15 14:38, Thomas Huth wrote: > On 11/12/15 01:15, Samuel Thibault wrote: >> From: Guillaume Subiron <maet...@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 <maet...@subiron.org> >> Signed-off-by: Samuel Thibault <samuel.thiba...@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