[quagga-dev 7268] Re: [PATCH 3/3] zebra: add interface rename support

paul at jakma.org paul at jakma.org
Fri Sep 4 21:26:14 BST 2009


On Wed, 5 Aug 2009, Stephen Hemminger wrote:

> This fixes issues with interfaces being renamed on Linux.

Could we possibly give more context on what issues are being 
addressed in future? :) I have to say I couldn't figure out 
exactly what was wrong.

Thanks though. Some questions about if_rename however.. See below.

>
> -  int index;
> -  uint32_t table;
> -  uint32_t metric;
> +  uint32_t index, table, metric;
>
>   void *dest;
>   void *gate;
> @@ -720,7 +697,7 @@ netlink_routing_table (struct sockaddr_n
>   src = NULL;
>
>   if (tb[RTA_OIF])
> -    index = *(int *) RTA_DATA (tb[RTA_OIF]);
> +    index = *(uint32_t *) RTA_DATA (tb[RTA_OIF]);

Seems a bit like a clean-up..


> +/* Interfaces can only be renamed when DOWN */
> +void
> +if_rename (struct interface *ifp, const char *name)
> +{
> +  struct interface *oifp;
> +
> +  UNSET_FLAG (ifp->status, ZEBRA_INTERFACE_ACTIVE);
> +  zebra_interface_delete_update (ifp);
> +  listnode_delete (iflist, ifp);
> +
> +  /* rename overlaps earlier interface */
> +  oifp = if_lookup_by_name(name);
> +  if (oifp)
> +    {
> +      ifp->status |= oifp->status; /* inherit config bits */
> +      if (oifp->ifindex != IFINDEX_INTERNAL)
> +	{
> +	  zlog_err ("interface %s rename to %s overlaps with index %d",
> +		    ifp->name, name, oifp->ifindex);
> +	  if_delete_update (oifp);
> +	}
> +      else if (IS_ZEBRA_DEBUG_KERNEL)
> +	zlog_debug ("interface %s index %d superseded by rename of %s",
> +		    oifp->name, oifp->ifindex, ifp->name);

So how does this happen? We've got interface X and Y, and Y gets 
renamed to X. So did X get implicitely deconfigured in the kernel? Or 
??

Also, why do we inherit some of the config from oifp, but not all?

> +      listnode_delete (iflist, oifp);
> +      XFREE (MTYPE_IF, oifp);

Shouldn't this call if_delete() instead? It seems like a couple of 
things get leaked here, no? E.g. the connected (struct list *), the 
oifp->info (struct zebra_if *) - which gets freed via the delete hook 
usually.

I'll change this to if_delete() in my apply for now.

> +    }
> +
> +  strncpy(ifp->name, name, INTERFACE_NAMSIZ);
> +  ifp->name[INTERFACE_NAMSIZ] = 0;
> +  listnode_add_sort (iflist, ifp);
> +
> +  zebra_interface_add_update (ifp);
> +
> +  SET_FLAG (ifp->status, ZEBRA_INTERFACE_ACTIVE);
> +
> +  if (ifp->connected)
> +    {
> +      struct connected *ifc;
> +      struct listnode *node;
> +      for (ALL_LIST_ELEMENTS_RO (ifp->connected, node, ifc))
> +	zebra_interface_address_add_update (ifp, ifc);
> +    }

Why not call if_add_update here? At least, it seems like there more 
stuff that needs to be done.

Also, can the interface be up? If so, don't we need to withdraw 
stuff?

Finally, bearing in mind that in rt_netlink.c you've changed the 
lookup from by-name to by-index, what happens if:

zebra config holds:

 	interface X
 	  <some configuration>

- Kernel creates an interface named Y
- Y gets renamed to X
- zebra looks up the ifindex, finds Y
- zebra does the rename Y->X, finds X as oifp and deletes it.

The old rename detection would have done:

- zebra looks up X, gets it with all the config
- zebra checks the ifindex and finds it belongs to Y, deletes Y.
- if_add_update done on X

I'm just wondering what the problem was with the old way and why this 
new way is better? In one we delete X, in the other Y (though, maybe 
we should set it to IFINDEX_INTERNAL instead of deleting...).

??

regards,
-- 
Paul Jakma	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
In Devon, Connecticut, it is unlawful to walk backwards after sunset.



More information about the Quagga-dev mailing list