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 <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)?
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
