Re: [Qemu-devel] [PATCH] slirp: Read host DNS config on demand

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Aug 02, 2009 at 10:03:10AM +0200, Jan Kiszka wrote:
> Michael S. Tsirkin wrote:
> > On Fri, Jul 31, 2009 at 06:10:22PM -0700, Ed Swierk wrote:
> >> Currently the qemu user-mode networking stack reads the host DNS
> >> configuration (/etc/resolv.conf or the Windows equivalent) only once
> >> when qemu starts.  This causes name lookups in the guest to fail if the
> >> host is moved to a different network from which the original DNS servers
> >> are unreachable, a common occurrence when the host is a laptop.
> >>
> >> This patch changes the slirp code to read the host DNS configuration on
> >> demand, caching the results for 10 seconds to avoid unnecessary overhead
> >> if name lookups occur in rapid succession.
> >>
> >> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> >> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Introducing a random delay until update might surprise users.
> > Would it make sense to use something like inotify instead?
> 
> IMHO, 10 s is below the user surprise threshold for a dynamically
> changing network link. Moreover, inotify does not exist on all platforms.
> 
> Let's keep things simple for the first step, we could still further
> improve it later on. 10 s are already an improvement over the current
> infinite timeout.
> 
> Jan

Maybe you are right ... OTOH it is at least consistently infinite :)

> > 
> >> ---
> >> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> >> index 95a4b39..751a8e2 100644
> >> --- a/slirp/ip_icmp.c
> >> +++ b/slirp/ip_icmp.c
> >> @@ -127,7 +127,8 @@ icmp_input(struct mbuf *m, int hlen)
> >>            slirp->vnetwork_addr.s_addr) {
> >>  	/* It's an alias */
> >>  	if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> >> -	  addr.sin_addr = dns_addr;
> >> +	  if (get_dns_addr(&addr.sin_addr) < 0)
> >> +	    addr.sin_addr = loopback_addr;
> >>  	} else {
> >>  	  addr.sin_addr = loopback_addr;
> >>  	}
> >> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> >> index 93087ed..67c70e3 100644
> >> --- a/slirp/libslirp.h
> >> +++ b/slirp/libslirp.h
> >> @@ -8,6 +8,8 @@
> >>  struct Slirp;
> >>  typedef struct Slirp Slirp;
> >>  
> >> +int get_dns_addr(struct in_addr *pdns_addr);
> >> +
> >>  Slirp *slirp_init(int restricted, struct in_addr vnetwork,
> >>                    struct in_addr vnetmask, struct in_addr vhost,
> >>                    const char *vhostname, const char *tftp_path,
> >> diff --git a/slirp/main.h b/slirp/main.h
> >> index 28d92d8..e87b068 100644
> >> --- a/slirp/main.h
> >> +++ b/slirp/main.h
> >> @@ -32,7 +32,6 @@ extern u_int curtime;
> >>  extern fd_set *global_readfds, *global_writefds, *global_xfds;
> >>  extern struct in_addr our_addr;
> >>  extern struct in_addr loopback_addr;
> >> -extern struct in_addr dns_addr;
> >>  extern char *username;
> >>  extern char *socket_path;
> >>  extern int towrite_max;
> >> diff --git a/slirp/slirp.c b/slirp/slirp.c
> >> index e883f82..987aad9 100644
> >> --- a/slirp/slirp.c
> >> +++ b/slirp/slirp.c
> >> @@ -29,8 +29,6 @@
> >>  
> >>  /* host address */
> >>  struct in_addr our_addr;
> >> -/* host dns address */
> >> -struct in_addr dns_addr;
> >>  /* host loopback address */
> >>  struct in_addr loopback_addr;
> >>  
> >> @@ -51,9 +49,12 @@ static int do_slowtimo;
> >>  TAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
> >>      TAILQ_HEAD_INITIALIZER(slirp_instances);
> >>  
> >> +struct in_addr dns_addr = { 0 };
> >> +u_int dns_addr_time = 0;
> >> +
> >>  #ifdef _WIN32
> >>  
> >> -static int get_dns_addr(struct in_addr *pdns_addr)
> >> +int get_dns_addr(struct in_addr *pdns_addr)
> >>  {
> >>      FIXED_INFO *FixedInfo=NULL;
> >>      ULONG    BufLen;
> >> @@ -61,6 +62,11 @@ static int get_dns_addr(struct in_addr *pdns_addr)
> >>      IP_ADDR_STRING *pIPAddr;
> >>      struct in_addr tmp_addr;
> >>  
> >> +    if (dns_addr.s_addr != 0 && (curtime - dns_addr_time) < 10000) {
> >> +        *pdns_addr = dns_addr;
> >> +        return 0;
> >> +    }
> >> +
> >>      FixedInfo = (FIXED_INFO *)GlobalAlloc(GPTR, sizeof(FIXED_INFO));
> >>      BufLen = sizeof(FIXED_INFO);
> >>  
> >> @@ -84,6 +90,8 @@ static int get_dns_addr(struct in_addr *pdns_addr)
> >>      pIPAddr = &(FixedInfo->DnsServerList);
> >>      inet_aton(pIPAddr->IpAddress.String, &tmp_addr);
> >>      *pdns_addr = tmp_addr;
> >> +    dns_addr = tmp_addr;
> >> +    dns_addr_time = curtime;
> >>      if (FixedInfo) {
> >>          GlobalFree(FixedInfo);
> >>          FixedInfo = NULL;
> >> @@ -98,7 +106,7 @@ static void winsock_cleanup(void)
> >>  
> >>  #else
> >>  
> >> -static int get_dns_addr(struct in_addr *pdns_addr)
> >> +int get_dns_addr(struct in_addr *pdns_addr)
> >>  {
> >>      char buff[512];
> >>      char buff2[257];
> >> @@ -106,6 +114,11 @@ static int get_dns_addr(struct in_addr *pdns_addr)
> >>      int found = 0;
> >>      struct in_addr tmp_addr;
> >>  
> >> +    if (dns_addr.s_addr != 0 && (curtime - dns_addr_time) < 10000) {
> >> +        *pdns_addr = dns_addr;
> >> +        return 0;
> >> +    }
> >> +
> >>      f = fopen("/etc/resolv.conf", "r");
> >>      if (!f)
> >>          return -1;
> >> @@ -120,8 +133,11 @@ static int get_dns_addr(struct in_addr *pdns_addr)
> >>              if (tmp_addr.s_addr == loopback_addr.s_addr)
> >>                  tmp_addr = our_addr;
> >>              /* If it's the first one, set it to dns_addr */
> >> -            if (!found)
> >> +            if (!found) {
> >>                  *pdns_addr = tmp_addr;
> >> +                dns_addr = tmp_addr;
> >> +                dns_addr_time = curtime;
> >> +            }
> >>  #ifdef DEBUG
> >>              else
> >>                  lprint(", ");
> >> @@ -177,11 +193,6 @@ static void slirp_init_once(void)
> >>      if (our_addr.s_addr == 0) {
> >>          our_addr = loopback_addr;
> >>      }
> >> -
> >> -    /* FIXME: This address may change during runtime */
> >> -    if (get_dns_addr(&dns_addr) < 0) {
> >> -        dns_addr = loopback_addr;
> >> -    }
> >>  }
> >>  
> >>  static void slirp_state_save(QEMUFile *f, void *opaque);
> >> diff --git a/slirp/socket.c b/slirp/socket.c
> >> index d8fbe89..207109c 100644
> >> --- a/slirp/socket.c
> >> +++ b/slirp/socket.c
> >> @@ -552,7 +552,8 @@ sosendto(struct socket *so, struct mbuf *m)
> >>  	    slirp->vnetwork_addr.s_addr) {
> >>  	  /* It's an alias */
> >>  	  if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> >> -	    addr.sin_addr = dns_addr;
> >> +	    if (get_dns_addr(&addr.sin_addr) < 0)
> >> +	      addr.sin_addr = loopback_addr;
> >>  	  } else {
> >>  	    addr.sin_addr = loopback_addr;
> >>  	  }
> >> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> >> index 51b3834..0417345 100644
> >> --- a/slirp/tcp_subr.c
> >> +++ b/slirp/tcp_subr.c
> >> @@ -340,7 +340,8 @@ int tcp_fconnect(struct socket *so)
> >>          slirp->vnetwork_addr.s_addr) {
> >>        /* It's an alias */
> >>        if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> >> -	addr.sin_addr = dns_addr;
> >> +	if (get_dns_addr(&addr.sin_addr) < 0)
> >> +	  addr.sin_addr = loopback_addr;
> >>        } else {
> >>  	addr.sin_addr = loopback_addr;
> >>        }
> >>
> >>
> >>
> 
>