lagg0.2 style vlans on lagg(4) interface

If one tries to use lagg0.2 style vlans on lagg0 interface
configured
from rc.conf it does't work.
The problem is that all of the cloned interfaces (lagg0 ,
lagg0.2, etc)
are created before any other interface configuration is
done,
and in this case lagg0 is created, then lagg0.2 is created.
But because lagg0 does not have any members yet, it's MAC
address
is all zeroes, and this is inherited by the vlan
interfaces.
At the next step the members are added to the lagg0
interface and it's
mac address gets set,
but the vlan interfaces never see this change and remain
with mac
address 00:00:00:00:00:00 and in unusable state.

Does it sound reasonable for the vlan(4) interfaces to
update their
mac address when their parent changes it.
(this should probably look more like the parent telling the
vlans
attached to it that it's mac has changed, but
i'm not sure if the parent knows about children interfaces)
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Wednesday 21 May 2008 19:31:46 Niki Denev wrote:
> If one tries to use lagg0.2 style vlans on lagg0
interface configured
> from rc.conf it does't work.
> The problem is that all of the cloned interfaces (lagg0
, lagg0.2, etc)
> are created before any other interface configuration is
done,
> and in this case lagg0 is created, then lagg0.2 is
created.
> But because lagg0 does not have any members yet, it's
MAC address
> is all zeroes, and this is inherited by the vlan
interfaces.
> At the next step the members are added to the lagg0
interface and it's
> mac address gets set,
> but the vlan interfaces never see this change and
remain with mac
> address 00:00:00:00:00:00 and in unusable state.
>
> Does it sound reasonable for the vlan(4) interfaces to
update their
> mac address when their parent changes it.
> (this should probably look more like the parent telling
the vlans
> attached to it that it's mac has changed, but
> i'm not sure if the parent knows about children
interfaces)

It doesn't (and shouldn't have to).  I'd simply add an 
EVENTHANDLER_INVOKE(ifaddr_event, ifp) to if_setlladdr() -
we do that for 
INET[6] address already.  Then vlan (and any other device
interested in 
LLaddress changes) can simply register to that eventhandler
and resync.

-- 
/"  Best regards,                      | mlaierfreebsd.org
 /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.l
ove2party.net/  | mlaierEFnet
/   ASCII Ribbon Campaign              | Against HTML Mail
and News
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Wed, May 21, 2008 at 8:44 PM, Max Laier <maxlove2party.net> wrote:
> On Wednesday 21 May 2008 19:31:46 Niki Denev wrote:
>> If one tries to use lagg0.2 style vlans on lagg0
interface configured
>> from rc.conf it does't work.
>> The problem is that all of the cloned interfaces
(lagg0 , lagg0.2, etc)
>> are created before any other interface
configuration is done,
>> and in this case lagg0 is created, then lagg0.2 is
created.
>> But because lagg0 does not have any members yet,
it's MAC address
>> is all zeroes, and this is inherited by the vlan
interfaces.
>> At the next step the members are added to the lagg0
interface and it's
>> mac address gets set,
>> but the vlan interfaces never see this change and
remain with mac
>> address 00:00:00:00:00:00 and in unusable state.
>>
>> Does it sound reasonable for the vlan(4) interfaces
to update their
>> mac address when their parent changes it.
>> (this should probably look more like the parent
telling the vlans
>> attached to it that it's mac has changed, but
>> i'm not sure if the parent knows about children
interfaces)
>
> It doesn't (and shouldn't have to).  I'd simply add an
> EVENTHANDLER_INVOKE(ifaddr_event, ifp) to
if_setlladdr() - we do that for
> INET[6] address already.  Then vlan (and any other
device interested in
> LLaddress changes) can simply register to that
eventhandler and resync.
>
> --
> /"  Best regards,                      |
mlaierfreebsd.org
>  /  Max Laier                          | ICQ
#67774661
>  X   http://pf4freebsd.l
ove2party.net/  | mlaierEFnet
> /   ASCII Ribbon Campaign              | Against HTML
Mail and News
>
>

Yes, you are absolutely right. I wasn't aware of
EVENTHANDLER(4).

Niki
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Wed, May 21, 2008 at 8:44 PM, Max Laier <maxlove2party.net> wrote:
>> It doesn't (and shouldn't have to).  I'd simply add
an
>> EVENTHANDLER_INVOKE(ifaddr_event, ifp) to
if_setlladdr() - we do that for
>> INET[6] address already.  Then vlan (and any other
device interested in
>> LLaddress changes) can simply register to that
eventhandler and resync.
>>

Hi,
Does this look like a reasonable solution?
It passes my limited testing.

Niki

diff -ur /usr/src/sys/net/if.c
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if.c
--- /usr/src/sys/net/if.c       2008-04-29
23:43:08.000000000 +0300
+++ /usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if.c
2008-05-21 22:12:33.989318352 +0300
 -2647,6
+2647,11 
                }
 #endif
        }
+       /*
+        * Notify interested parties (clonable children like
if_vlan(4))
+        * about the link layer address change
+        */
+       EVENTHANDLER_INVOKE(ifaddr_event, ifp);
        return (0);
 }

diff -ur /usr/src/sys/net/if_vlan.c
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if_vlan.c
--- /usr/src/sys/net/if_vlan.c  2007-10-28
18:24:16.000000000 +0200
+++
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if_vlan.c
2008-05-21 23:24:13.322156460 +0300
 -137,6
+137,7 
 static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q Virtual
LAN Interface");

 static eventhandler_tag ifdetach_tag;
+static eventhandler_tag ifaddr_tag;

 /*
  * We have a global mutex, that is used to serialize
configuration
 -518,6
+519,33 
 /* For if_link_state_change() eyes only... */
 extern void (*vlan_link_state_p)(struct ifnet *, int);

+/*
+ * Update vlan interface link layer address on
+ * parent interface link layer address change.
+ */
+static int
+vlan_lladdr_update(void *arg __unused, struct ifnet *ifp)
+{
+       struct ifvlantrunk *trunk = ifp->if_vlantrunk;
+       struct ifvlan *ifv;
+       int i;
+
+       TRUNK_LOCK(trunk);
+#ifdef VLAN_ARRAY
+       for (i = 0; i < VLAN_ARRAY_SIZE; i++)
+               if (trunk->vlans[i] != NULL) {
+                       ifv = trunk->vlans[i];
+#else
+       for (i = 0; i < (1 << trunk->hwidth);
i++) {
+               LIST_FOREACH(ifv, &trunk->hash[i],
ifv_list)
+#endif
+                       bcopy(IF_LLADDR(ifp),
IF_LLADDR(ifv->ifv_ifp),
+                           ETHER_ADDR_LEN);
+       }
+       TRUNK_UNLOCK(trunk);
+       return 0;
+}
+
 static int
 vlan_modevent(module_t mod, int type, void *data)
 {
 -528,6
+556,10 
                    vlan_ifdetach, NULL,
EVENTHANDLER_PRI_ANY);
                if (ifdetach_tag == NULL)
                        return (ENOMEM);
+               ifaddr_tag =
EVENTHANDLER_REGISTER(ifaddr_event,
+                   vlan_lladdr_update, NULL,
EVENTHANDLER_PRI_ANY);
+               if (ifaddr_tag == NULL)
+                       return (ENOMEM);
                VLAN_LOCK_INIT();
                vlan_input_p = vlan_input;
                vlan_link_state_p = vlan_link_state;
 -546,6
+578,7 
        case MOD_UNLOAD:
                if_clone_detach(&vlan_cloner);
               
EVENTHANDLER_DEREGISTER(ifnet_departure_event,
ifdetach_tag);
+               EVENTHANDLER_DEREGISTER(ifaddr_event,
ifaddr_tag);
                vlan_input_p = NULL;
                vlan_link_state_p = NULL;
                vlan_trunk_cap_p = NULL;
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Wednesday 21 May 2008 22:44:42 Niki Denev wrote:
> On Wed, May 21, 2008 at 8:44 PM, Max Laier <maxlove2party.net> wrote:
> >> It doesn't (and shouldn't have to).  I'd
simply add an
> >> EVENTHANDLER_INVOKE(ifaddr_event, ifp) to
if_setlladdr() - we do
> >> that for INET[6] address already.  Then vlan
(and any other device
> >> interested in LLaddress changes) can simply
register to that
> >> eventhandler and resync.
>
> Hi,
> Does this look like a reasonable solution?
> It passes my limited testing.

Looks good, though I'd probably move up the _INVOKE to
before the ARPs are 
sent out.  Probably between twiddling the hardware and
sending ARPs 
(though that needs an else-case if the interface is still
down).  In fact 
the ARPs could be sent from an event hook, too.  This would
get rid of 
INET specific code in the LL-interface code.

> Niki
>
> diff -ur /usr/src/sys/net/if.c
> /usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if.c
> --- /usr/src/sys/net/if.c       2008-04-29
23:43:08.000000000 +0300
> +++
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if.c
> 2008-05-21 22:12:33.989318352 +0300
>  -2647,6 +2647,11 
>                 }
>  #endif
>         }
> +       /*
> +        * Notify interested parties (clonable children
like
> if_vlan(4)) +        * about the link layer address
change
> +        */
> +       EVENTHANDLER_INVOKE(ifaddr_event, ifp);
>         return (0);
>  }
>
> diff -ur /usr/src/sys/net/if_vlan.c
>
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if_vlan.c
> --- /usr/src/sys/net/if_vlan.c  2007-10-28
18:24:16.000000000 +0200
> +++
/usr/src/.zfs/snapshot/vlan_lladdr_patch/sys/net/if_vlan.c
> 2008-05-21 23:24:13.322156460 +0300
>  -137,6 +137,7 
>  static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q
Virtual LAN
> Interface");
>
>  static eventhandler_tag ifdetach_tag;
> +static eventhandler_tag ifaddr_tag;
>
>  /*
>   * We have a global mutex, that is used to serialize
configuration
>  -518,6 +519,33 
>  /* For if_link_state_change() eyes only... */
>  extern void (*vlan_link_state_p)(struct ifnet *,
int);
>
> +/*
> + * Update vlan interface link layer address on
> + * parent interface link layer address change.
> + */
> +static int
> +vlan_lladdr_update(void *arg __unused, struct ifnet
*ifp)
> +{
> +       struct ifvlantrunk *trunk =
ifp->if_vlantrunk;
> +       struct ifvlan *ifv;
> +       int i;
> +
> +       TRUNK_LOCK(trunk);
> +#ifdef VLAN_ARRAY
> +       for (i = 0; i < VLAN_ARRAY_SIZE; i++)
> +               if (trunk->vlans[i] != NULL) {
> +                       ifv = trunk->vlans[i];
> +#else
> +       for (i = 0; i < (1 <<
trunk->hwidth); i++) {
> +               LIST_FOREACH(ifv,
&trunk->hash[i], ifv_list)
> +#endif
> +                       bcopy(IF_LLADDR(ifp),
IF_LLADDR(ifv->ifv_ifp),
> +                           ETHER_ADDR_LEN);
> +       }
> +       TRUNK_UNLOCK(trunk);
> +       return 0;
> +}
> +
>  static int
>  vlan_modevent(module_t mod, int type, void *data)
>  {
>  -528,6 +556,10 
>                     vlan_ifdetach, NULL,
EVENTHANDLER_PRI_ANY);
>                 if (ifdetach_tag == NULL)
>                         return (ENOMEM);
> +               ifaddr_tag =
EVENTHANDLER_REGISTER(ifaddr_event,
> +                   vlan_lladdr_update, NULL,
EVENTHANDLER_PRI_ANY);
> +               if (ifaddr_tag == NULL)
> +                       return (ENOMEM);
>                 VLAN_LOCK_INIT();
>                 vlan_input_p = vlan_input;
>                 vlan_link_state_p = vlan_link_state;
>  -546,6 +578,7 
>         case MOD_UNLOAD:
>                 if_clone_detach(&vlan_cloner);
>                
EVENTHANDLER_DEREGISTER(ifnet_departure_event,
> ifdetach_tag); +              
EVENTHANDLER_DEREGISTER(ifaddr_event,
> ifaddr_tag); vlan_input_p = NULL;
>                 vlan_link_state_p = NULL;
>                 vlan_trunk_cap_p = NULL;



-- 
/"  Best regards,                      | mlaierfreebsd.org
 /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.l
ove2party.net/  | mlaierEFnet
/   ASCII Ribbon Campaign              | Against HTML Mail
and News
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Thu, May 22, 2008 at 12:05 AM, Max Laier <maxlove2party.net> wrote:
> Looks good, though I'd probably move up the _INVOKE to
before the ARPs are
> sent out.  Probably between twiddling the hardware and
sending ARPs
> (though that needs an else-case if the interface is
still down).  In fact
> the ARPs could be sent from an event hook, too.  This
would get rid of
> INET specific code in the LL-interface code.
>

I thought about moving it up too, but in this case isn't it
going to
be called only if the interface is up (IFF_UP)?

Here is an updated version that makes vlan_lladdr_update()
check if
ifp->if_vlantrunk exists,
because otherwise we get a panic if there is a change of the
link
layer address on different interface that
does not have vlan children.

diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if.c
/usr/src/sys/net/if.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if.c	2008-04-29
23:43:08.000000000 +0300
+++ /usr/src/sys/net/if.c	2008-05-21 23:48:59.621157217
+0300
 -2647,6
+2647,12 
 		}
 #endif
 	}
+	/*
+	 * Notify interested parties (clonable children like
if_vlan(4))
+	 * about the link layer address change
+	 */
+	EVENTHANDLER_INVOKE(ifaddr_event, ifp);
+
 	return (0);
 }

diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c
/usr/src/sys/net/if_vlan.c
---
/usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c	2007-10-28
18:24:16.000000000 +0200
+++ /usr/src/sys/net/if_vlan.c	2008-05-22 00:18:57.128613715
+0300
 -137,6
+137,7 
 static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q Virtual
LAN Interface");

 static eventhandler_tag ifdetach_tag;
+static eventhandler_tag ifaddr_tag;

 /*
  * We have a global mutex, that is used to serialize
configuration
 -518,6
+519,35 
 /* For if_link_state_change() eyes only... */
 extern	void (*vlan_link_state_p)(struct ifnet *, int);

+/*
+ * Update vlan interface link layer address on
+ * parent interface link layer address change.
+ */
+static int
+vlan_lladdr_update(void *arg __unused, struct ifnet *ifp)
+{
+	struct ifvlantrunk *trunk = ifp->if_vlantrunk;
+	struct ifvlan *ifv;
+	int i;
+
+	if (trunk) {
+		TRUNK_LOCK(trunk);
+#ifdef VLAN_ARRAY
+		for (i = 0; i < VLAN_ARRAY_SIZE; i++)
+			if (trunk->vlans[i] != NULL) {
+				ifv = trunk->vlans[i];
+#else
+		for (i = 0; i < (1 << trunk->hwidth); i++) {
+			LIST_FOREACH(ifv, &trunk->hash[i], ifv_list)
+#endif
+				bcopy(IF_LLADDR(ifp), IF_LLADDR(ifv->ifv_ifp),
+					ETHER_ADDR_LEN);
+		}
+		TRUNK_UNLOCK(trunk);
+	}
+	return (0);
+}
+
 static int
 vlan_modevent(module_t mod, int type, void *data)
 {
 -528,6
+558,10 
 		    vlan_ifdetach, NULL, EVENTHANDLER_PRI_ANY);
 		if (ifdetach_tag == NULL)
 			return (ENOMEM);
+		ifaddr_tag = EVENTHANDLER_REGISTER(ifaddr_event,
+			vlan_lladdr_update, NULL, EVENTHANDLER_PRI_ANY);
+		if (ifaddr_tag == NULL)
+			return (ENOMEM);
 		VLAN_LOCK_INIT();
 		vlan_input_p = vlan_input;
 		vlan_link_state_p = vlan_link_state;
 -546,6
+580,7 
 	case MOD_UNLOAD:
 		if_clone_detach(&vlan_cloner);
 		EVENTHANDLER_DEREGISTER(ifnet_departure_event,
ifdetach_tag);
+		EVENTHANDLER_DEREGISTER(ifaddr_event, ifaddr_tag);
 		vlan_input_p = NULL;
 		vlan_link_state_p = NULL;
 		vlan_trunk_cap_p = NULL;
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Wednesday 21 May 2008 23:22:52 Niki Denev wrote:
> On Thu, May 22, 2008 at 12:05 AM, Max Laier <maxlove2party.net> wrote:
> > Looks good, though I'd probably move up the
_INVOKE to before the
> > ARPs are sent out.  Probably between twiddling the
hardware and
> > sending ARPs (though that needs an else-case if
the interface is
                 
^-------------------------------------------------
> > still down).  In fact the ARPs could be sent from
an event hook, too.
    ---------^
> >  This would get rid of INET specific code in the
LL-interface code.
>
> I thought about moving it up too, but in this case
isn't it going to
> be called only if the interface is up (IFF_UP)?

See above.

> Here is an updated version that makes
vlan_lladdr_update() check if
> ifp->if_vlantrunk exists,
> because otherwise we get a panic if there is a change
of the link
> layer address on different interface that
> does not have vlan children.
>
> diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if.c
/usr/src/sys/net/if.c
> ---
/usr/src/.zfs/snapshot/orig/sys/net/if.c	2008-04-29
> 23:43:08.000000000 +0300 +++
/usr/src/sys/net/if.c	2008-05-21
> 23:48:59.621157217 +0300
>  -2647,6 +2647,12 
>  		}
>  #endif
>  	}
> +	/*
> +	 * Notify interested parties (clonable children like
if_vlan(4))
> +	 * about the link layer address change
> +	 */
> +	EVENTHANDLER_INVOKE(ifaddr_event, ifp);
> +
>  	return (0);
>  }
>
> diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c
> /usr/src/sys/net/if_vlan.c
> ---
/usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c	2007-10-28
> 18:24:16.000000000 +0200
> +++ /usr/src/sys/net/if_vlan.c	2008-05-22
00:18:57.128613715 +0300
>  -137,6 +137,7 
>  static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q
Virtual LAN
> Interface");
>
>  static eventhandler_tag ifdetach_tag;
> +static eventhandler_tag ifaddr_tag;
>
>  /*
>   * We have a global mutex, that is used to serialize
configuration
>  -518,6 +519,35 
>  /* For if_link_state_change() eyes only... */
>  extern	void (*vlan_link_state_p)(struct ifnet *,
int);
>
> +/*
> + * Update vlan interface link layer address on
> + * parent interface link layer address change.
> + */
> +static int
> +vlan_lladdr_update(void *arg __unused, struct ifnet
*ifp)
> +{
> +	struct ifvlantrunk *trunk = ifp->if_vlantrunk;
> +	struct ifvlan *ifv;
> +	int i;
> +
> +	if (trunk) {
> +		TRUNK_LOCK(trunk);
> +#ifdef VLAN_ARRAY
> +		for (i = 0; i < VLAN_ARRAY_SIZE; i++)
> +			if (trunk->vlans[i] != NULL) {
> +				ifv = trunk->vlans[i];
> +#else
> +		for (i = 0; i < (1 << trunk->hwidth);
i++) {
> +			LIST_FOREACH(ifv, &trunk->hash[i],
ifv_list)
> +#endif
> +				bcopy(IF_LLADDR(ifp), IF_LLADDR(ifv->ifv_ifp),
> +					ETHER_ADDR_LEN);
> +		}
> +		TRUNK_UNLOCK(trunk);
> +	}
> +	return (0);
> +}
> +
>  static int
>  vlan_modevent(module_t mod, int type, void *data)
>  {
>  -528,6 +558,10 
>  		    vlan_ifdetach, NULL, EVENTHANDLER_PRI_ANY);
>  		if (ifdetach_tag == NULL)
>  			return (ENOMEM);
> +		ifaddr_tag = EVENTHANDLER_REGISTER(ifaddr_event,
> +			vlan_lladdr_update, NULL, EVENTHANDLER_PRI_ANY);
> +		if (ifaddr_tag == NULL)
> +			return (ENOMEM);
>  		VLAN_LOCK_INIT();
>  		vlan_input_p = vlan_input;
>  		vlan_link_state_p = vlan_link_state;
>  -546,6 +580,7 
>  	case MOD_UNLOAD:
>  		if_clone_detach(&vlan_cloner);
>  		EVENTHANDLER_DEREGISTER(ifnet_departure_event,
ifdetach_tag);
> +		EVENTHANDLER_DEREGISTER(ifaddr_event, ifaddr_tag);
>  		vlan_input_p = NULL;
>  		vlan_link_state_p = NULL;
>  		vlan_trunk_cap_p = NULL;


-- 
/"  Best regards,                      | mlaierfreebsd.org
 /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.l
ove2party.net/  | mlaierEFnet
/   ASCII Ribbon Campaign              | Against HTML Mail
and News
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Thu, May 22, 2008 at 12:32 AM, Max Laier <maxlove2party.net> wrote:
> On Wednesday 21 May 2008 23:22:52 Niki Denev wrote:
>> On Thu, May 22, 2008 at 12:05 AM, Max Laier
<maxlove2party.net> wrote:
>> > Looks good, though I'd probably move up the
_INVOKE to before the
>> > ARPs are sent out.  Probably between twiddling
the hardware and
>> > sending ARPs (though that needs an else-case
if the interface is
>                 
^-------------------------------------------------
>> > still down).  In fact the ARPs could be sent
from an event hook, too.
>    ---------^
>> >  This would get rid of INET specific code in
the LL-interface code.
>>
>> I thought about moving it up too, but in this case
isn't it going to
>> be called only if the interface is up (IFF_UP)?
>
> See above.
>

Oh, I've missed that.

So, generating ARPs from an event handler looks to like the
best solution.
But in that case what would be the correct place for it?
if_ethersubr.c and the others (fw,fddi,arc...) that may use
ARP?

Niki
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Thursday 22 May 2008 00:14:37 Niki Denev wrote:
> On Thu, May 22, 2008 at 12:32 AM, Max Laier <maxlove2party.net> wrote:
> > On Wednesday 21 May 2008 23:22:52 Niki Denev
wrote:
> >> On Thu, May 22, 2008 at 12:05 AM, Max Laier
<maxlove2party.net> 
wrote:
> >> > Looks good, though I'd probably move up
the _INVOKE to before the
> >> > ARPs are sent out.  Probably between
twiddling the hardware and
> >> > sending ARPs (though that needs an
else-case if the interface is
> >
> >                 
^-------------------------------------------------
> >
> >> > still down).  In fact the ARPs could be
sent from an event hook,
> >> > too.
> >
> >    ---------^
> >
> >> >  This would get rid of INET specific code
in the LL-interface
> >> > code.
> >>
> >> I thought about moving it up too, but in this
case isn't it going to
> >> be called only if the interface is up
(IFF_UP)?
> >
> > See above.
>
> Oh, I've missed that.
>
> So, generating ARPs from an event handler looks to like
the best
> solution. But in that case what would be the correct
place for it?
> if_ethersubr.c and the others (fw,fddi,arc...) that may
use ARP?

I think if_ether.c would be good, where arp_ifinit() is.

-- 
/"  Best regards,                      | mlaierfreebsd.org
 /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.l
ove2party.net/  | mlaierEFnet
/   ASCII Ribbon Campaign              | Against HTML Mail
and News
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

On Thu, May 22, 2008 at 1:41 AM, Max Laier <maxlove2party.net> wrote:
> I think if_ether.c would be good, where arp_ifinit()
is.
>

Here is what i've come up with (some comments after the
patch) :

diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if.c
/usr/src/sys/net/if.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if.c    2008-04-29
23:43:08.000000000 +0300
+++ /usr/src/sys/net/if.c       2008-05-22
04:56:42.652730327 +0300
 -2636,17
+2636,12 
                        (*ifp->if_ioctl)(ifp,
SIOCSIFFLAGS, (caddr_t)&ifr);
                        IFF_UNLOCKGIANT(ifp);
                }
-#ifdef INET
-               /*
-                * Also send gratuitous ARPs to notify other
nodes about
-                * the address change.
-                */
-               TAILQ_FOREACH(ifa, &ifp->if_addrhead,
ifa_link) {
-                       if (ifa->ifa_addr->sa_family
== AF_INET)
-                               arp_ifinit(ifp, ifa);
-               }
-#endif
        }
+       /*
+        * Notify interested parties about the link layer
address change.
+        */
+       EVENTHANDLER_INVOKE(ifaddr_event, ifp);
+
        return (0);
 }

diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_ethersubr.c
/usr/src/sys/net/if_ethersubr.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if_ethersubr.c 
2008-05-21
23:29:07.899194044 +0300
+++ /usr/src/sys/net/if_ethersubr.c     2008-05-22
06:38:35.698083868 +0300
 -48,6
+48,7 
 #include <sys/socket.h>
 #include <sys/sockio.h>
 #include <sys/sysctl.h>
+#include <sys/eventhandler.h>

 #include <net/if.h>
 #include <net/if_arp.h>
 -99,6
+100,8 

 #include <security/mac/mac_framework.h>

+static eventhandler_tag ifaddr_tag;
+
 /* netgraph node hooks for ng_ether(4) */
 void   (*ng_ether_input_p)(struct ifnet *ifp, struct mbuf
**mp);
 void   (*ng_ether_input_orphan_p)(struct ifnet *ifp, struct
mbuf *m);
 -910,6
+913,13 
                if_printf(ifp, "Ethernet address:
%6Dn", lla, ":");
        if (ifp->if_flags & IFF_NEEDSGIANT)
                if_printf(ifp, "if_start running
deferred for Giantn");
+#ifdef INET
+       /* Register address change hook */
+       ifaddr_tag = EVENTHANDLER_REGISTER(ifaddr_event,
arp_gratuit,
+               NULL, EVENTHANDLER_PRI_ANY);
+       //if (ifaddr_tag == NULL)
+       //      return (ENOMEM);
+#endif
 }

 /*
 -924,6
+934,8 
                (*ng_ether_detach_p)(ifp);
        }

+       EVENTHANDLER_DEREGISTER(ifaddr_event, ifaddr_tag);
+
        bpfdetach(ifp);
        if_detach(ifp);
 }
diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c
/usr/src/sys/net/if_vlan.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c      
2007-10-28
18:24:16.000000000 +0200
+++ /usr/src/sys/net/if_vlan.c  2008-05-22
00:18:57.128613715 +0300
 -137,6
+137,7 
 static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q Virtual
LAN Interface");

 static eventhandler_tag ifdetach_tag;
+static eventhandler_tag ifaddr_tag;

 /*
  * We have a global mutex, that is used to serialize
configuration
 -518,6
+519,35 
 /* For if_link_state_change() eyes only... */
 extern void (*vlan_link_state_p)(struct ifnet *, int);

+/*
+ * Update vlan interface link layer address on
+ * parent interface link layer address change.
+ */
+static int
+vlan_lladdr_update(void *arg __unused, struct ifnet *ifp)
+{
+       struct ifvlantrunk *trunk = ifp->if_vlantrunk;
+       struct ifvlan *ifv;
+       int i;
+
+       if (trunk) {
+               TRUNK_LOCK(trunk);
+#ifdef VLAN_ARRAY
+               for (i = 0; i < VLAN_ARRAY_SIZE; i++)
+                       if (trunk->vlans[i] != NULL) {
+                               ifv = trunk->vlans[i];
+#else
+               for (i = 0; i < (1 <<
trunk->hwidth); i++) {
+                       LIST_FOREACH(ifv,
&trunk->hash[i], ifv_list)
+#endif
+                               bcopy(IF_LLADDR(ifp),
IF_LLADDR(ifv->ifv_ifp),
+                                       ETHER_ADDR_LEN);
+               }
+               TRUNK_UNLOCK(trunk);
+       }
+       return (0);
+}
+
 static int
 vlan_modevent(module_t mod, int type, void *data)
 {
 -528,6
+558,10 
                    vlan_ifdetach, NULL,
EVENTHANDLER_PRI_ANY);
                if (ifdetach_tag == NULL)
                        return (ENOMEM);
+               ifaddr_tag =
EVENTHANDLER_REGISTER(ifaddr_event,
+                       vlan_lladdr_update, NULL,
EVENTHANDLER_PRI_ANY);
+               if (ifaddr_tag == NULL)
+                       return (ENOMEM);
                VLAN_LOCK_INIT();
                vlan_input_p = vlan_input;
                vlan_link_state_p = vlan_link_state;
 -546,6
+580,7 
        case MOD_UNLOAD:
                if_clone_detach(&vlan_cloner);
               
EVENTHANDLER_DEREGISTER(ifnet_departure_event,
ifdetach_tag);
+               EVENTHANDLER_DEREGISTER(ifaddr_event,
ifaddr_tag);
                vlan_input_p = NULL;
                vlan_link_state_p = NULL;
                vlan_trunk_cap_p = NULL;
diff -ur /usr/src/.zfs/snapshot/orig/sys/netinet/if_ether.c
/usr/src/sys/netinet/if_ether.c
--- /usr/src/.zfs/snapshot/orig/sys/netinet/if_ether.c 
2007-10-07
23:44:22.000000000 +0300
+++ /usr/src/sys/netinet/if_ether.c     2008-05-22
06:41:34.966106759 +0300
 -956,6
+956,16 
        ifa->ifa_flags |= RTF_CLONING;
 }

+void
+arp_gratuit(void *arg __unused, struct ifnet *ifp)
+{
+       struct ifaddr *ifa;
+       TAILQ_FOREACH(ifa, &ifp->if_addrhead,
ifa_link) {
+               if (ifa->ifa_addr->sa_family ==
AF_INET)
+                       arp_ifinit(ifp, ifa);
+       }
+}
+
 static void
 arp_init(void)
 {
diff -ur /usr/src/.zfs/snapshot/orig/sys/netinet/if_ether.h
/usr/src/sys/netinet/if_ether.h
--- /usr/src/.zfs/snapshot/orig/sys/netinet/if_ether.h 
2005-02-22
15:04:03.000000000 +0200
+++ /usr/src/sys/netinet/if_ether.h     2008-05-22
05:24:06.749938771 +0300
 -113,6
+113,7 
                struct mbuf *m, struct sockaddr *dst, u_char
*desten);
 void   arp_ifinit(struct ifnet *, struct ifaddr *);
 void   arp_ifinit2(struct ifnet *, struct ifaddr *, u_char
*);
+void   arp_gratuit(void *, struct ifnet *);
 #endif

 #endif

I think arp_gratuit() needs a better name.
Is if_ethersubr.c:ether_ifattach() good place to register
the EVENT hook?
And if yes, what would be the best way to handle failure to
register
the hook, as the function is void?
Should I worry about that, or just print a warning message
and continue?


Niki
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"