On 2011-07-30 11:09, Jan Kiszka wrote:
> On 2011-07-29 19:34, Jan Kiszka wrote:
>> On 2011-07-29 18:25, Fabien Chouteau wrote:
>>> This patch adds a simple ARP table in Slirp and also adds handling of
>>> gratuitous ARP requests.
>>>
>>> Signed-off-by: Fabien Chouteau <chout...@adacore.com>
>>> ---
>>> Makefile.objs | 2 +-
>>> slirp/arp_table.c | 50 ++++++++++++++++++++++++++++++++++++++++++
>>> slirp/bootp.c | 23 ++++++++++++------
>>> slirp/slirp.c | 63
>>> +++++++++++++---------------------------------------
>>> slirp/slirp.h | 50 +++++++++++++++++++++++++++++++++++++++--
>>> 5 files changed, 129 insertions(+), 59 deletions(-)
>>> create mode 100644 slirp/arp_table.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 6991a9f..0c10557 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -151,7 +151,7 @@ common-obj-y += qemu-timer.o qemu-timer-common.o
>>>
>>> slirp-obj-y = cksum.o if.o ip_icmp.o ip_input.o ip_output.o
>>> slirp-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o
>>> tcp_output.o
>>> -slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o
>>> +slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o arp_table.o
>>> common-obj-$(CONFIG_SLIRP) += $(addprefix slirp/, $(slirp-obj-y))
>>>
>>> # xen backend driver support
>>> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
>>> new file mode 100644
>>> index 0000000..5d7404b
>>> --- /dev/null
>>> +++ b/slirp/arp_table.c
>>> @@ -0,0 +1,50 @@
>>> +#include "slirp.h"
>>> +
>>> +void arp_table_add(ArpTable *arptbl,
>>> + int ip_addr,
>>> + uint8_t ethaddr[ETH_ALEN])
>>
>> I still prefer the condensed formatting, but that's a minor nit.
>>
>>> +{
>>> + int i;
>>> +
>>> + DEBUG_CALL("arp_table_add");
>>> + DEBUG_ARG("ip = 0x%x", ip_addr);
>>> + DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> + ethaddr[0], ethaddr[1], ethaddr[2],
>>> + ethaddr[3], ethaddr[4], ethaddr[5]));
>>> +
>>> + /* Search for an entry */
>>> + for (i = 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip != 0; i++) {
>>
>> Missed that on round #1: Why treating 0.0.0.0 special? If ip_addr can be
>> zero, the current logic will append every "update" of that address as a
>> new entry.
>>
>>> + if (arptbl->table[i].ar_sip == ip_addr) {
>>> + /* Update the entry */
>>> + memcpy(arptbl->table[i].ar_sha, ethaddr, ETH_ALEN);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + /* No entry found, create a new one */
>>> + arptbl->table[arptbl->next_victim].ar_sip = ip_addr;
>>> + memcpy(arptbl->table[arptbl->next_victim].ar_sha, ethaddr, ETH_ALEN);
>>> + arptbl->next_victim = (arptbl->next_victim + 1) % ARP_TABLE_SIZE;
>>> +}
>>> +
>>> +bool arp_table_search(ArpTable *arptbl,
>>> + int in_ip_addr,
>>> + uint8_t out_ethaddr[ETH_ALEN])
>>> +{
>>> + int i;
>>> +
>>> + DEBUG_CALL("arp_table_search");
>>> + DEBUG_ARG("ip = 0x%x", in_ip_addr);
>>> +
>>> + for (i = 0; i < ARP_TABLE_SIZE; i++) {
>>> + if (arptbl->table[i].ar_sip == in_ip_addr) {
>>> + memcpy(out_ethaddr, arptbl->table[i].ar_sha, ETH_ALEN);
>>> + DEBUG_ARGS((dfd, " found hw addr =
>>> %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> + out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
>>> + out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
>>> + return 1;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> diff --git a/slirp/bootp.c b/slirp/bootp.c
>>> index 1eb2ed1..07cbb80 100644
>>> --- a/slirp/bootp.c
>>> +++ b/slirp/bootp.c
>>> @@ -149,6 +149,7 @@ static void bootp_reply(Slirp *slirp, const struct
>>> bootp_t *bp)
>>> struct in_addr preq_addr;
>>> int dhcp_msg_type, val;
>>> uint8_t *q;
>>> + uint8_t client_ethaddr[ETH_ALEN];
>>>
>>> /* extract exact DHCP msg type */
>>> dhcp_decode(bp, &dhcp_msg_type, &preq_addr);
>>> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct
>>> bootp_t *bp)
>>> if (dhcp_msg_type != DHCPDISCOVER &&
>>> dhcp_msg_type != DHCPREQUEST)
>>> return;
>>> - /* XXX: this is a hack to get the client mac address */
>>> - memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
>>> +
>>> + /* Get client's hardware address from bootp request */
>>> + memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
>>>
>>> m = m_get(slirp);
>>> if (!m) {
>>> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct
>>> bootp_t *bp)
>>>
>>> if (dhcp_msg_type == DHCPDISCOVER) {
>>> if (preq_addr.s_addr != htonl(0L)) {
>>> - bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> + bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>> if (bc) {
>>> daddr.sin_addr = preq_addr;
>>> }
>>> }
>>> if (!bc) {
>>> new_addr:
>>> - bc = get_new_addr(slirp, &daddr.sin_addr,
>>> slirp->client_ethaddr);
>>> + bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
>>> if (!bc) {
>>> DPRINTF("no address left\n");
>>> return;
>>> }
>>> }
>>> - memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> + memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>> } else if (preq_addr.s_addr != htonl(0L)) {
>>> - bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> + bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>> if (bc) {
>>> daddr.sin_addr = preq_addr;
>>> - memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> + memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>> } else {
>>> daddr.sin_addr.s_addr = 0;
>>> }
>>> @@ -209,6 +211,11 @@ static void bootp_reply(Slirp *slirp, const struct
>>> bootp_t *bp)
>>> }
>>> }
>>>
>>> + if (daddr.sin_addr.s_addr != 0) {
>>> + /* Update ARP table for this IP address */
>>> + arp_table_add(&slirp->arp_table, daddr.sin_addr.s_addr,
>>> client_ethaddr);
>>
>> Here you prevent that arp_table_add is called with a zero IP, but not in
>> arp_input below. Likely harmless, but at least inconsistent.
>>
>
> Unfortunately, this patch also has a more sever issues: it breaks DHCP e.g.
>
> Slirp's bootp sends out all replies, also acks and offers, as
> broadcasts. Normal servers already use the clients IP address as
> destination here.Obviously, using a broadcast address on return is valid as well. So this is no slirp bug, it's purely an ARP table lookup issue introduced by this patch. > > Even if bootp is fixed, you still lack logic to deal with special > addresses in your arp table lookup. At least broadcasts need to be > handled, I think other multicasts aren't supported by slirp anyway. > Jan
signature.asc
Description: OpenPGP digital signature
