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

Everton Marques everton.marques at gmail.com
Mon Sep 22 20:43:55 BST 2014


Christian,

On Mon, Sep 22, 2014 at 1:56 PM, Christian Franke <nobody at nowhere.ws> wrote:

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

Ok, removed.


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

Ok.
Renamed reset_iface_addresses to non-static if_connected_reset_all(void),
and moved to lib/if.[hc].
It seems to work properly with list_delete_all_node():

git diff mrib reconnect lib/if.c
diff --git a/lib/if.c b/lib/if.c
index 18e2fb3..e15bf4c 100644
--- a/lib/if.c
+++ b/lib/if.c
@@ -138,6 +138,18 @@ if_create (const char *name, int namelen)
   return ifp;
 }

+void
+if_connected_reset_all (void) {
+  struct listnode  *ifnode;
+  struct interface *ifp;
+
+  zlog_warn("%s: resetting all interface addresses", __func__);
+
+  for (ALL_LIST_ELEMENTS_RO(iflist, ifnode, ifp)) {
+    list_delete_all_node(ifp->connected);
+  }
+}
+
 /* Delete interface structure. */
 void
 if_delete_retain (struct interface *ifp)



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

Done.


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

Sorry, I don't quite follow.
Anyway, automatic reset of connected addresses has been removed from
zclient code.
pimd is now explicitly calling removal of connected address within
zclient_broken callback.


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

Ok, done.

Please take another look at the reconnect branch.

>
Thanks,
Everton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20140922/e67e6fb9/attachment.html>


More information about the Quagga-dev mailing list