[quagga-dev 270] Re: ripd fix to handle interface aliases

Paul Jakma paul at clubi.ie
Tue Sep 30 21:07:40 BST 2003


Hi Sowmini,

On Mon, 29 Sep 2003 sowmini.varadhan at sun.com wrote:

> 
> Hi,
> 
> I have a patch to enable ripd to handle interface aliases,

ah excellent. Note that I've just made some changes which
rip_interface.c at least - a revert of a far too troublesome patch.  
You probably need to resync.

> so that it sends out updates on each connected network as per RFC
> 2453. This patch has been tested on Solaris 9, and I've also tested
> for compilation on linux. I'm attaching the diffs below, but since
> the patch involves 2 new files (solaris specific), it's probably
> easier to download
> http://www.cs.utk.edu/~varadhan/quagga/patch.tar.gz to look at the
> patch.

ok.

> All files can also be viewed at http://www.cs.utk.edu/~varadhan/quagga/
> 
> Comments are appreciated!

sure. see inline below.

> 
> --Sowmini
> 
> -----------------diffs below cut here-----------------------------------
> 
> ===================================================================
> RCS file: configure.ac,v
> retrieving revision 1.30
> retrieving revision 1.31
> diff -u -r1.30 -r1.31
> --- /tmp/T0GQaGJo	Mon Sep 29 13:55:39 2003
> +++ /tmp/T1HQaGJo	Mon Sep 29 13:55:39 2003
> @@ -425,23 +425,43 @@
>  if test "$netlink" = yes; then
>    AC_MSG_RESULT(netlink)
>    IF_METHOD=if_netlink.o
> +  IOCTL_METHOD=ioctl.o

why do we need ioctl if we have netlink (and hence presumably use 
if_netlink not if_ioctl)? whats missing here?

>    elif test "$opsys" = "openbsd";then
>      AC_MSG_RESULT(openbsd)
>      IF_METHOD=if_ioctl.o
> +    IOCTL_METHOD=ioctl.o

why is this needed? if_ioctl needs ioctl to be linked unconditionally 
no? if_ioctl implies ioctl?

> ===================================================================
> RCS file: if.h,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -u -r1.9 -r1.10
> --- /tmp/T00xaiKo	Mon Sep 29 13:55:50 2003
> +++ /tmp/T11xaiKo	Mon Sep 29 13:55:50 2003
> @@ -81,6 +81,11 @@
>    /* Interface index. */
>    unsigned int ifindex;
>  
> +#ifdef SUNOS_5
> +  /* Interface family */
> +  sa_family_t af;
> +#endif /* SUNOS_5 */
> +

hmmm.... you should keep the address family as a zebra^Wquagga afi_t, 
not sa_family_t. further, how can the interface have an address 
family? these are associated with an address, which are kept in the 
connected list (struct connected references the struct interface).

>    /* Interface MTU. */
> -  int mtu;
> +  int mtu;  /* ipv4 mtu */
> +#ifdef SOLARIS_IPV6
> +  int mtu6; /* ipv6 mtu */
> +#endif

mtu's are a hardware thing - surely the MTU on an interface applies 
to all address families using that interface? (ie why do you want an 
IPv6 specific MTU field? (yes the comment is misleading :) )).

> -  for (node = listhead (ifp->connected); node; nextnode (node))

try and use the LIST_LOOP macro.

> +  if (if_pointopoint)
> +    p = (struct prefix_ipv4 *) connected->destination;
> +  else
> +    p = (struct prefix_ipv4 *) connected->address;
>  
> -      connected = getdata (node);
> -      p = (struct prefix_ipv4 *) connected->address;
> +  addr = p->prefix;
>  
> -      if (p->family == AF_INET)
> -	{
> -	  addr = p->prefix;
> +#ifdef SUNOS_5
> +  /*
> +   * Note that we intentionally reset IP_XMIT_IF to zero if
> +   * we're doing multicast.  The kernel ignores IP_MULTICAST_IF
> +   * if IP_XMIT_IF is set, and we can't deal with alias source
> +   * addresses without it.
> +   */
> +  setsockopt(sock, IPPROTO_IP, IP_XMIT_IF, &ifindex, sizeof(ifindex)) ;
> +#endif

hmm... can you explain the above a wee bit please? i'm not sure why 
this needs to be SunOS specific. whats going on here, and what are 
you trying to work around?

>  
> -	  if (setsockopt_multicast_ipv4 (sock, IP_MULTICAST_IF,
> -					 addr, 0, ifp->ifindex) < 0) 
> -	    {
> -	      zlog_warn ("Can't setsockopt IP_MULTICAST_IF to fd %d", sock);
> -	      return;
> -	    }
> +  if (setsockopt_multicast_ipv4 (sock, IP_MULTICAST_IF,
> +				 addr, 0, 0) < 0) 
> +    {

hmm.. see comment above, whats going on?

> -    if (ripd_privs.change (ZPRIVS_LOWER))
> -        zlog_err ("rip_interface_multicast_set: could not lower privs");
> +  if (ripd_privs.change (ZPRIVS_LOWER))
> +    zlog_err ("rip_interface_multicast_set: could not lower privs");

diff -uwb would be better - to avoid the whitespace churn. (if you 
prod me enough i'll run indent on the file after committing the 
actual changes).

>  void rip_output_process (struct interface *, struct prefix *,
> -			 struct sockaddr_in *, int, u_char);
> +			 struct sockaddr_in *, int, u_char, 
> +                         struct prefix_ipv4 *);

this is to pass out the source address right? you cant determine that 
from the interface? or from the sockaddr_in 'to' parameter? (ok - 
that can be passed in as NULL). Perhaps it'd be better to distinguish 
between physical interfaces and 'rip interfaces' - ie create a struct 
rip_interface to hold RIP-specific information (ie neighbours and 
source addresses, etc. for each subnet attached to an interface on 
which RIP is active) - a la ospfd's struct ospf_interface? keep a 
list of such structs in (struct interface *)->info

@@ -2336,16 +2340,31 @@
>  			   ifp->ifindex);
>  	    }
>  
> -	  /* If there is no version configuration in the interface,
> -             use rip's version setting. */
> -	  {
> +          /* send update on each connected network */
> +	  for (ifnode = listhead (ifp->connected); ifnode; nextnode (ifnode))
> +	    {
> +	      struct prefix_ipv4 *ifaddr;
> +              struct connected *connected;
> +          
> +
> +	      /* If there is no version configuration in the interface,
> +                 use rip's version setting. */
>  	      int vsend = ((ri->ri_send == RI_RIP_UNSPEC) ?
>  			   rip->version_send : ri->ri_send);
> +
> +      	      connected = getdata (ifnode);
> +              ifaddr = (struct prefix_ipv4 *) connected->address;
> +
> +	      if (ifaddr->family != AF_INET)
> +		continue;

why would ripd even have added an address to the connected list if it 
was not AF_INET? alternatively, see comment above about a possible 
list of rip_interface's for each interface.

> -  return rip_send_packet ((caddr_t) &rip_packet, sizeof (rip_packet), to, ifp);
> +  /* send request on each connected network */

> --- /tmp/T0AxaGMo	Mon Sep 29 13:56:27 2003
> +++ /tmp/T1BxaGMo	Mon Sep 29 13:56:27 2003
> @@ -14,14 +14,15 @@
>  rtread_method = @RTREAD_METHOD@
>  kernel_method = @KERNEL_METHOD@
>  other_method = @OTHER_METHOD@
> +ioctl_method = @IOCTL_METHOD@
>  
>  otherobj = $(ipforward) $(if_method) $(if_proc) $(rt_method) \
> -	$(rtread_method) $(kernel_method) $(other_method)
> +	$(rtread_method) $(kernel_method) $(other_method) $(ioctl_method)

why would we need multiple ioctl_method's? (this ties in with the 
configure.ac changes).

regards,
-- 
Paul Jakma	paul at clubi.ie	paul at jakma.org	Key ID: 64A2FF6A
	warning: do not ever send email to spam at dishone.st
Fortune:
"MSDOS didn't get as bad as it is overnight -- it took over ten years
of careful development."
(By dmeggins at aix1.uottawa.ca)



More information about the Quagga-dev mailing list