Re: [Dnsmasq-discuss] [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.

2015-08-11 Thread Simon Kelley 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256



On 10/08/15 22:29, Laine Stump wrote:
 On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote:
 This is a fix for commit
 db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process
 exits without waiting for DAD, this is dnsmasq daemon's task. So
 we periodically poll the kernel using netlink and check whether
 there are any IPv6 addresses assigned to bridge which have
 'tentative' state. After DAD is finished, execution continues. I
 guess that is what dnsmasq was assumed to do.
 
 Since the comments in our code imply that dnsmasq should be waiting
 for DAD to complete prior to daemonizing, before pushing a fix like
 this I'd like to find out from the dnsmasq folks if we are
 erroneously relying on nonexistent dnsmasq behavior, or if maybe
 there is a bug in some version of dnsmasq.
 
 Simon (or other dnsmasq people) - when dnsmasq is run with
 enable-ra, does it make sure it completes DAD prior to
 daemonizing? Or does libvirt need to do this extra polling to
 assure that DAD has completed? (or maybe there's some other config
 parameter we need to add?)
 
 

Dnsmasq doesn't wait for DAD to complete before returning. Internally,
it know is DAD is still happening on an interface, as it needs to
delay calling bind() until after it's complete. It would, therefore be
relatively simple to add this behaviour, but it's not a complete
solution, since new interfaces can appear _after_ dnsmasq has
completed start-up.

Having libvirt do its own checks whenever it creates an interface
might therefore be a cleaner way of doing things, but I don't have an
objection to changing dnsmasq behaviour if there's a good reason why
that's not sensible.


Cheers,

Simon.



 
 --- Difference to v2: Moved to virnetdev.
 
 src/libvirt_private.syms|   1 + src/network/bridge_driver.c |
 35 +- src/util/virnetdev.c| 160
  src/util/virnetdev.h
 |   2 + 4 files changed, 197 insertions(+), 1 deletion(-)
 
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms 
 index 0517c24..fa9e1c1 100644 --- a/src/libvirt_private.syms +++
 b/src/libvirt_private.syms @@ -1776,6 +1776,7 @@
 virNetDevSetRcvMulti; virNetDevSetupControl; virNetDevSysfsFile; 
 virNetDevValidateConfig; +virNetDevWaitDadFinish;
 
 
 # util/virnetdevbandwidth.h diff --git
 a/src/network/bridge_driver.c b/src/network/bridge_driver.c index
 3d6721b..2172a3d 100644 --- a/src/network/bridge_driver.c +++
 b/src/network/bridge_driver.c @@ -2026,6 +2026,33 @@
 networkAddRouteToBridge(virNetworkObjPtr network, }
 
 static int +networkWaitDadFinish(virNetworkObjPtr network) +{ +
 virNetworkIpDefPtr ipdef; +virSocketAddrPtr *addrs = NULL; +
 size_t i; +int ret; +for (i = 0; + (ipdef =
 virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
 i++) {} + +if (i == 0) +return 0; +if
 (VIR_ALLOC_N(addrs, i)) +return -1; + +for (i = 0; +
 (ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
 i++) { +addrs[i] = ipdef-address; +} + +ret =
 virNetDevWaitDadFinish(addrs, i); +VIR_FREE(addrs); +
 return ret; +} + +static int 
 networkStartNetworkVirtual(virNetworkDriverStatePtr driver, 
 virNetworkObjPtr network) { @@ -2159,7 +2186,13 @@
 networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if
 (v6present  networkStartRadvd(driver, network)  0) goto err4;
 
 -/* DAD has happened (dnsmasq waits for it), dnsmasq is now
 bound to the +/* dnsmasq main process does not wait for DAD
 to complete, + * so we need to wait for it ourselves. +
 */ +if (v6present  networkWaitDadFinish(network)  0) +
 goto err4; + +/* DAD has happened, dnsmasq is now bound to
 the * bridge's IPv6 address, so we can now set the dummy tun
 down. */ if (tapfd = 0) { diff --git a/src/util/virnetdev.c
 b/src/util/virnetdev.c index 1e20789..c81342a 100644 ---
 a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -96,6 +96,7
 @@ VIR_LOG_INIT(util.netdev); # define
 FEATURE_BIT_IS_SET(blocks, index, field)\ 
 (FEATURE_WORD(blocks, index, field)  FEATURE_FIELD_FLAG(index)) 
 #endif +# define IP_BUF_SIZE 4096
 
 typedef enum { VIR_MCAST_TYPE_INDEX_TOKEN, @@ -1219,6 +1220,103
 @@ int virNetDevClearIPAddress(const char *ifname, return ret; }
 
 +/* return whether there is a known address with 'tentative' flag
 set */ +static int +virNetDevParseDadStatus(struct nlmsghdr *nlh,
 int len, +virSocketAddrPtr *addrs, size_t
 count) +{ +struct ifaddrmsg *ifaddrmsg_ptr; +unsigned int
 ifaddrmsg_len; +struct rtattr *rtattr_ptr; +size_t i; +
 struct in6_addr *addr; +for (; NLMSG_OK(nlh, len); nlh =
 NLMSG_NEXT(nlh, len)) { +if (NLMSG_PAYLOAD(nlh, 0) 
 sizeof(struct ifaddrmsg)) { +/* Message without
 payload is the last one. */ +break; +} + +
 ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh); +if

Re: [Dnsmasq-discuss] [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.

2015-08-11 Thread Laine Stump 
On 08/11/2015 04:14 AM, Simon Kelley wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA256



 On 10/08/15 22:29, Laine Stump wrote:
 On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote:
 This is a fix for commit
 db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process
 exits without waiting for DAD, this is dnsmasq daemon's task. So
 we periodically poll the kernel using netlink and check whether
 there are any IPv6 addresses assigned to bridge which have
 'tentative' state. After DAD is finished, execution continues. I
 guess that is what dnsmasq was assumed to do.
 Since the comments in our code imply that dnsmasq should be waiting
 for DAD to complete prior to daemonizing, before pushing a fix like
 this I'd like to find out from the dnsmasq folks if we are
 erroneously relying on nonexistent dnsmasq behavior, or if maybe
 there is a bug in some version of dnsmasq.

 Simon (or other dnsmasq people) - when dnsmasq is run with
 enable-ra, does it make sure it completes DAD prior to
 daemonizing? Or does libvirt need to do this extra polling to
 assure that DAD has completed? (or maybe there's some other config
 parameter we need to add?)


 Dnsmasq doesn't wait for DAD to complete before returning. Internally,
 it know is DAD is still happening on an interface, as it needs to
 delay calling bind() until after it's complete. It would, therefore be
 relatively simple to add this behaviour, but it's not a complete
 solution, since new interfaces can appear _after_ dnsmasq has
 completed start-up.

 Having libvirt do its own checks whenever it creates an interface
 might therefore be a cleaner way of doing things, but I don't have an
 objection to changing dnsmasq behaviour if there's a good reason why
 that's not sensible.

No need for dnsmasq to change its behavior. I just wanted to make sure
that it was the comment in libvirt that was wrong, and not a regression
in dnsmasq. Based on your answer, it appears that this was a
misunderstanding by the original author of the libvirt code, so it is
something that libvirt needs to fix.

Thanks for the quick response!

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.

2015-08-10 Thread Laine Stump 
On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote:
 This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970
 dnsmasq main process exits without waiting for DAD, this is dnsmasq
 daemon's task. So we periodically poll the kernel using netlink and
 check whether there are any IPv6 addresses assigned to bridge
 which have 'tentative' state. After DAD is finished, execution continues.
 I guess that is what dnsmasq was assumed to do.

Since the comments in our code imply that dnsmasq should be waiting for
DAD to complete prior to daemonizing, before pushing a fix like this I'd
like to find out from the dnsmasq folks if we are erroneously relying on
nonexistent dnsmasq behavior, or if maybe there is a bug in some version
of dnsmasq.

Simon (or other dnsmasq people) - when dnsmasq is run with enable-ra,
does it make sure it completes DAD prior to daemonizing? Or does libvirt
need to do this extra polling to assure that DAD has completed? (or
maybe there's some other config parameter we need to add?)


 ---
 Difference to v2: Moved to virnetdev.

  src/libvirt_private.syms|   1 +
  src/network/bridge_driver.c |  35 +-
  src/util/virnetdev.c| 160 
 
  src/util/virnetdev.h|   2 +
  4 files changed, 197 insertions(+), 1 deletion(-)

 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index 0517c24..fa9e1c1 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -1776,6 +1776,7 @@ virNetDevSetRcvMulti;
  virNetDevSetupControl;
  virNetDevSysfsFile;
  virNetDevValidateConfig;
 +virNetDevWaitDadFinish;


  # util/virnetdevbandwidth.h
 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 3d6721b..2172a3d 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -2026,6 +2026,33 @@ networkAddRouteToBridge(virNetworkObjPtr network,
  }

  static int
 +networkWaitDadFinish(virNetworkObjPtr network)
 +{
 +virNetworkIpDefPtr ipdef;
 +virSocketAddrPtr *addrs = NULL;
 +size_t i;
 +int ret;
 +for (i = 0;
 + (ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, i));
 + i++) {}
 +
 +if (i == 0)
 +return 0;
 +if (VIR_ALLOC_N(addrs, i))
 +return -1;
 +
 +for (i = 0;
 + (ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, i));
 + i++) {
 +addrs[i] = ipdef-address;
 +}
 +
 +ret = virNetDevWaitDadFinish(addrs, i);
 +VIR_FREE(addrs);
 +return ret;
 +}
 +
 +static int
  networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
 virNetworkObjPtr network)
  {
 @@ -2159,7 +2186,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr 
 driver,
  if (v6present  networkStartRadvd(driver, network)  0)
  goto err4;

 -/* DAD has happened (dnsmasq waits for it), dnsmasq is now bound to the
 +/* dnsmasq main process does not wait for DAD to complete,
 + * so we need to wait for it ourselves.
 + */
 +if (v6present  networkWaitDadFinish(network)  0)
 +goto err4;
 +
 +/* DAD has happened, dnsmasq is now bound to the
   * bridge's IPv6 address, so we can now set the dummy tun down.
   */
  if (tapfd = 0) {
 diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
 index 1e20789..c81342a 100644
 --- a/src/util/virnetdev.c
 +++ b/src/util/virnetdev.c
 @@ -96,6 +96,7 @@ VIR_LOG_INIT(util.netdev);
  # define FEATURE_BIT_IS_SET(blocks, index, field)\
  (FEATURE_WORD(blocks, index, field)  FEATURE_FIELD_FLAG(index))
  #endif
 +# define IP_BUF_SIZE 4096

  typedef enum {
  VIR_MCAST_TYPE_INDEX_TOKEN,
 @@ -1219,6 +1220,103 @@ int virNetDevClearIPAddress(const char *ifname,
  return ret;
  }

 +/* return whether there is a known address with 'tentative' flag set */
 +static int
 +virNetDevParseDadStatus(struct nlmsghdr *nlh, int len,
 +virSocketAddrPtr *addrs, size_t count)
 +{
 +struct ifaddrmsg *ifaddrmsg_ptr;
 +unsigned int ifaddrmsg_len;
 +struct rtattr *rtattr_ptr;
 +size_t i;
 +struct in6_addr *addr;
 +for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
 +if (NLMSG_PAYLOAD(nlh, 0)  sizeof(struct ifaddrmsg)) {
 +/* Message without payload is the last one. */
 +break;
 +}
 +
 +ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh);
 +if (!(ifaddrmsg_ptr-ifa_flags  IFA_F_TENTATIVE)) {
 +/* Not tentative: we are not interested in this entry. */
 +continue;
 +}
 +
 +ifaddrmsg_len = IFA_PAYLOAD(nlh);
 +rtattr_ptr = (struct rtattr *) IFA_RTA(ifaddrmsg_ptr);
 +for (; RTA_OK(rtattr_ptr, ifaddrmsg_len);
 +rtattr_ptr = RTA_NEXT(rtattr_ptr, ifaddrmsg_len)) {
 +if (RTA_PAYLOAD(rtattr_ptr) != sizeof(struct in6_addr)) {
 +/* No address: ignore. */
 +continue;
 +}