Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-08-01 Thread Fabien Chouteau
On 30/07/2011 11:19, Jan Kiszka wrote:
 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 000..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.

OK I'll change it to keep consistent style.

Unfortunately, there's nothing on this subject in the CODING_STYLE...


 +{
 +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.

Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.

 diff --git a/slirp/bootp.c b/slirp/bootp.c
 index 1eb2ed1..07cbb80 100644
 --- a/slirp/bootp.c
 +++ b/slirp/bootp.c
 @@ -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.

I will handle this case directly in arp_table_add to get a consistent behavior.


 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.



That's right, I will add broadcast handling in the next version.

Thanks for your review.

-- 
Fabien Chouteau



Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-08-01 Thread Jan Kiszka
On 2011-08-01 17:03, Fabien Chouteau wrote:
 On 30/07/2011 11:19, Jan Kiszka wrote:
 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 000..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.
 
 OK I'll change it to keep consistent style.
 
 Unfortunately, there's nothing on this subject in the CODING_STYLE...

We should add a section on consistency - but I guess that will always
remain a subjective matter. :)

 

 +{
 +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.
 
 Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.

Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
up in the ARP table.

Jan



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-08-01 Thread Fabien Chouteau
On 01/08/2011 17:11, Jan Kiszka wrote:
 On 2011-08-01 17:03, Fabien Chouteau wrote:
 On 30/07/2011 11:19, Jan Kiszka wrote:
 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 000..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.

 OK I'll change it to keep consistent style.

 Unfortunately, there's nothing on this subject in the CODING_STYLE...

 We should add a section on consistency - but I guess that will always
 remain a subjective matter. :)

Indeed, I bet we can find in Qemu an example of every C coding style...




 +{
 +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.

 Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.

 Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
 up in the ARP table.


Great, so I will just prevent insertion and search of 0.0.0.0/8 addresses.

Regards,

-- 
Fabien Chouteau



Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-07-30 Thread Jan Kiszka
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 000..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);
   

Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-07-30 Thread Jan Kiszka
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 000..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);
 +

Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

2011-07-29 Thread Jan Kiszka
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 000..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)) {