I object to these patches.
the idea is good but these patches are misguided..

Yevmenkin, Maksim N, CSCIO wrote:
> Hello All,
> i was working on integration of Ethernet TAP driver and NETGRAPH
> and found strange thing. the problem is that NG_ETHER nodes do not
> detach correctly when interface is gone. i was taking a very quick
> look at it, and, it seems to me that we are missing one reference
> to a node. i think it is ng_name_node/ng_unname pair.

This is quite possible because until recently interfaces could never
be removed. Therefore the act of removing a node was really
just a case of RESETTING the node. It was not removed.

> quick patch (works for NG_ETHER, but i did not have time to look
> at all NG_XXXX modules) available at
> http://home.earthlink.net/~evmax/ng_ether-diffs.tar.gz
> this problem could, possibly, affect other modules.
> after patch i was able to:
> - load Ethernet TAP driver
> - create several virtual Ethernet interfaces
> - load NG_ETHER module (and check nodes via ngctl)
> - unload Ethernet TAP driver (all virtual Ethernet interfaces are gone)
> - check that all NG_ETHER nodes are gone (via ngctl)
> - unload NG_ETHER module

it's rathe rude to do this in such an un-nested manner, but 
yes it would be good for this to work.

my comments on you patches follow:

ng_base.c   Wed Jul 12 22:54:09 2000
*** 632,638 ****
        if (node->name) {
                FREE(node->name, M_NETGRAPH);
                node->name = NULL;
!               ng_unref(node);
--- 632,638 ----
        if (node->name) {
                FREE(node->name, M_NETGRAPH);
                node->name = NULL;
!               node->refs--;

never never never decrement refs without checking
that is has not gone to 0. If it has gone to 0 the object
MUST be freed.!!!!!!! That is what ng_unref is for.
If you do not want it to be freed than you should add a reference
which represents the reference that you have to it. That way you can
guarantee that it will never go to 0. NEVER EVER EVER subvert a
reference count. (you go blind if you do that)

*** ng_ether.c.old      Wed Jul 12 22:53:38 2000
--- ng_ether.c  Wed Jul 12 22:55:41 2000
*** 287,297 ****
--- 287,299 ----
        if (node == NULL)               /* no node (why not?), ignore */
                  /* break all links to other nodes */
+       node->flags |= NG_INVALID;
        IFP2NG(ifp) = NULL;             /* detach node from interface */
        priv = node->private;           /* free node private info */
        bzero(priv, sizeof(*priv));
        FREE(priv, M_NETGRAPH);
        node->private = NULL;
+       ng_unname(node);                /* unname node */
        ng_unref(node);                 /* free node itself */

Ok, so the idea is that the actual underlying interface
is going away, and that you want the node to go away too.
The correct way is to signal to ng_ether_rmnode() that 
it SHOULD remove the node.

 At present this code assumes that the ethernet interface 
is permenent, and that the ng_ether node should thus also 
be persistant. What you need is a way for it to distinguish 
between the case where it should not remove the node, and the case
where the interface is doomed, and it should remove the node.
A flag somewhere would suffice.

It seems to me from a quick look at the code that
is in the -current tree, that there is some confusion
about device node removal as applied to ethernet interfaces.

Archie's changes (when he applies them)
will give a clearer picture at to how this should be done.
I suggest that you hold off until his patches are added because
it will have an effect. It should be done any day now.

The way you propose to do it will not meet my design criterea
for netgraph.

      __--_|\  Julian Elischer
     /       \ [EMAIL PROTECTED]
    (   OZ    ) World tour 2000
     ;_.---._/  presently in:  Budapest

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to