[prev in list] [next in list] [prev in thread] [next in thread] 

List:       quagga-dev
Subject:    [quagga-dev 271] Re: ripd fix to handle interface aliases
From:       sowmini.varadhan () sun ! com
Date:       2003-09-30 20:39:54
[Download RAW message or body]


Paul,

thanks for the prompt feedback!

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

Ok, will do.

> >  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?

Short answer: you'd still need some functions like if_set_flags, that are
defined in ioctl.c. Longer answer below...

> > +    IOCTL_METHOD=ioctl.o
> 
> why is this needed? if_ioctl needs ioctl to be linked unconditionally 
> no? if_ioctl implies ioctl?

With my changes, I created a new file ioctl_solaris.c that has
the solaris specific version (SIOCGLIF*) of the system calls that are found
in ioctl.c. All other systems should continue to link in ioctl.o,
except for solaris 9 and higher systems (which have ipv6, and need
the GLIF version of the ioctls) 

Prior to my changes, the configure.ac script linked in either 
{if_netlink.o and ioctl.o} or {if_ioctl.o and ioctl.o}. 
After my changes, this becomes
{if_netlink.o and ioctl.o}  on linux,
or {if_ioctl.o and ioctl.o} on most other systems,
or {if_ioctl_solaris.o and ioctl_solaris.o} on solaris 9 and higher.

> > +#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).

Solaris has a peculiar convention for interfaces- there is one
virtual interface per ip address, so that the output of ifconfig -a
can show something like

hme0: flags=1000843<UP,BROADCAST,RUNNING,MULTICAST,IPv4> mtu 1500 index 2
        inet 10.10.1.2 netmask ffffffc0 broadcast....
        ether 8:0:20:c9:b9:1b 
hme0:1: flags=1000843<UP,BROADCAST,RUNNING,MULTICAST,IPv4> mtu 1500 index 2
        inet 192.168.0.1 netmask ffff0000 broadcast ...
hme0: flags=2000841<UP,RUNNING,MULTICAST,IPv6> mtu 1500 index 2
        ether 8:0:20:c9:b9:1b 
        inet6 fe80::a00:20ff:fec9:b91b/10 
hme0:1: flags=2080841<UP,RUNNING,MULTICAST,ADDRCONF,IPv6> mtu 1500 index 2
        inet6 fec0:a08:3002:3:a00:20ff:fec9:b91b/64 

i.e., there are 2 interfaces called "hme0", and 2 called "hme0:1".
It gets more complicated when you do ioctls- you'd have to do  the 
ioctl on an ipv4 or ipv6 socket for with interface name "hme0" to get
some info like the mtu which might not be the same on the ipv4/ipv6 interface.

> > +  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 :) )).

You could use ifconfig to artificially set the mtu (which could affect
the fragmentation at the ip level, if you set the mtu to be lower than
the hardware mtu). Moreover, you could set this to a different value
for the ipv4 and ipv6 interface.

> 
> > -  for (node = listhead (ifp->connected); node; nextnode (node))
> 
> try and use the LIST_LOOP macro.

Ok.

> > +#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?

Solaris is wierd, and the kernel does some unexpected things with 
IP_XMIT_IF.


> 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).

Ok.. I'll keep that in mind for next time.

> 
> >  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). 

What if the interface has multiple addresses (in the example above,
the interface had 10.10.1.2 and 192.168.0.1) and we are sending
to 224.0.0.9, but want to make sure we send it on both the 10.10.1.0/25
as well as the 192.168.0.0/16 network by using the source address
that we have on each network? Also, we'd have to poison routes appropriately
(192.168.0.0/16 on the 10.10.1.0/25 network and vice-versa), which
is why I had to move the source-address selection to be above
the code the rip_output_process() in the call-chain.

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

The addresses are collected when zebra does the SIOCGIFCONF/SIOCGLIFCONF
ioctl and sends this down to ripd. Once I enabled HAVE_IPV6, zebra
sends off these messages to all its listeners, ripd picks up the info
and stashes it away as a "connected" network. Maybe it should quietly
drop the ipv6 info on the floor. let me think about it.


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

Let me know if the other explanation was unclear...

Thanks for the comments.. I'll go through them and incorporate
any action-items while I wait for you to parse this..

--Sowmini

PS: btw, I meant to send this with the earlier patch, but here's a
summary of changes I made:

new files
- zebra/ioctl_solaris.c
- zebra/if_ioctl_solaris.c

modified files
- configure.ac
  - See if SIOCGLIF* ioctls are available and cause *solaris.c files to be
    compiled in.
  - added IOCTL_METHOD

- zebra/Makefile.am
  - added IOCTL_METHOD

- lib/if.h
  - added ifmtu, if_af

- zebra/ioctl.h
  - Added solaris specific #defines.

- zebra/ipforward_solaris.c
  - fix typo: ipforwarding, not ipfowarding.

- ripd/rip_interface.c
  - reorganized rip_interface_multicast_set so that it can be called
    repeatedly for aliased interfaces (on multiple networks).

- ripd/ripd.c
  - rip_send_packet: use rip->sock for mcast sends, instead of creating
    one socket per send.
  - send source addr to rip_update_interface
  - rip_update_process should send an update on every connected network
    for each interface.
  - rip_request_send should send a request on every connected network
    for each interface

- ripd/ripd.h
  changed prototype of rip_interface_multicast_set



[prev in list] [next in list] [prev in thread] [next in thread]