[quagga-dev 11514] Re: [PATCH] pimd merge

Christian Franke nobody at nowhere.ws
Mon Sep 22 17:56:25 BST 2014


On 09/16/2014 08:47 PM, Everton Marques wrote:
> 2/3 reconnect - Branched from mrib, adding only zclient reconnection changes

> +++ b/lib/zclient.c
> @@ -54,12 +54,14 @@ zclient_new ()
>    zclient = XCALLOC (MTYPE_ZCLIENT, sizeof (struct zclient));
>  
>    zclient->ibuf = stream_new (ZEBRA_MAX_PACKET_SIZ);
>    zclient->obuf = stream_new (ZEBRA_MAX_PACKET_SIZ);
>    zclient->wb = buffer_new(0);
>  
> +  zclient->zclient_broken = NULL;
> +
>    return zclient;
>  }

This is not really necessary. zclient is allocated with XCALLOC, so all
fields are initialized to zero as it is. (The other callbacks are not
explicitly initialized either)

> +void
> +if_connected_reset(struct interface *ifp)
> +{
> +  list_free(ifp->connected);
> +  ifp->connected = list_new();
> +  ifp->connected->del = (void (*) (void *)) connected_free;
> +}

I think this will leak all the "struct connected" objects in
ifp->connected as it just frees the list and not its contents.
list_delete should work better than list_free.

Looking at the linklist API, I wonder whether the whole
if_connected_reset function couldn't be replaced with a call to
list_delete_all_node.

> +static void reset_iface_addresses() {

This should be "static void reset_iface_addresses(void)", otherwise it
creates a compiler warning:

zclient.c:115:13: warning: function declaration isn't a prototype

> +  struct listnode  *ifnode;
> +  struct interface *ifp;
> +
> +  zlog_warn("%s %s: resetting all interface addresses",
> +           __FILE__, __PRETTY_FUNCTION__);

As a minor nitpick, I would prefer __func__ over __PRETTY_FUNCTION__.
It's what is used in other places and Quagga is C code after all. Maybe
also drop the __FILE__ part to make it more consistent with the rest of
the logging in zebra.

> +
> +  for (ALL_LIST_ELEMENTS_RO(iflist, ifnode, ifp)) {
> +    if_connected_reset(ifp);
> +  }
> +}

On a high level, it seems to me that an application using this won't
notice if an interface has been removed while it was disconnected from
zebra.

An implementation issue which I see here is that this code assumes that
iflist is initialized to something sane. If an application doesn't make
use of lib/if, iflist will not be initialized.

This could be worked around by initializing iflist to NULL. However,
with the current API, the zclient part of the library does all work via
explicitly defined callbacks and it's the responsibility of the
application to make the calls into the lib/if subsystem if it wants to
use that subsystem. So it might be more consistent to remove the call to
reset_iface_addresses from zclient_stop and to have any application that
requires it make that call itself, either from zclient_broken or
possibly later, after connection has been reestablished.

-Christian




More information about the Quagga-dev mailing list