Mailing List Archive

I'd like to add, having a list that "allows dups" leads to a whole
bunch of questions.

True.

What are the semantics associated with "listnode_delete"?
It seems to delete just the first instance.

A good point. But I believe the way forward is documenting the
current behavior, and treading extremely lightly on changing it.

What are the semantics of "listnode_find" (is the caller expected to
march down the list and diddle all of the node entries that match
the key?)?

One could also argue that a caller that needs to do this should define
an appropriate comparison function. In the ripngd case, the
comparison function is explicitly only on next hop, since that's
apparently what the protocol requires. There is nothing wrong or even
unusual in the ripngd case with having multiple entries with the same
next hop.

> IMO, adding dups is just memory leakage, and sounds like poor design.
> ymmv.

In the ripngd case, there is no evidence that this leads to a memory
leak.

--
Greg Troxel <gdt@ir.bbn.com>

Gilad Arnold wrote:
> Hasso Tepper wrote:
> > for (n = list->head; n; n = n->next)
> > {
> > - if ((*list->cmp) (val, n->data) == 0)
> > - return;
> > - if ((*list->cmp) (val, n->data) < 0)
> > + if ((*list->cmp) (val, n->data) <= 0)
> > {
>
> Just one question: why would you change the comparison operator
> from '<' to '<='? Before your change, a duplicate element would
> have been added right after those which already exist in the list,
> by that maintaining some sort of chronological sense within the
> list elements. After your change, it would precede any prior equal
> elements.

I have no time to check at the moment (I have to run) but can you look
at code more? I remember that before that change new node was added
to the end of list, not after these elements with which cmp returned
0.

--
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator

Hasso Tepper wrote:

> I have no time to check at the moment (I have to run) but can you look
> at code more? I remember that before that change new node was added
> to the end of list, not after these elements with which cmp returned
> 0.

No, it would be added to the end of the list only if no greater element
was found after the equal elements... (in this case, clearly the tail of
the list and the tail of the equal elements sub-list are the same
location ;-> )

IOW, I would simply go back to the original behavior, which you
virtually did (apart from relocation of mutual malloc/assignment
statements, a good practice).

Gilad

>
> Yes, but is everybody doing this? I'm not sure zebra is.
>
> --Sowmini

Ok, to answer my own question for the if_create()->listnode_add_sort()
calls, I cscoped the latest code base, and
looks like this problem has been fixed in zebra because ifm_read
checks if_lookup_by_name() first, before calling if_create() (which
is the function that does listnode_add_sort())

However, just from what I see via cscope, I'm not sure if ospf_vl_new()
needs some more checks to avoid duplicate entries. ospf_vl_new()
always calls if_create, and never checks if the interface already
exists.

The semantics of the functions in linklist.c really needs more
documentation.

--Sowmini

The semantics of the functions in linklist.c really needs more
documentation.

I just committed some comments for linklist_add_sorted and some in
linklist.h.

From a quick glance, I don't see any other functions that are
particularly likely to be confusing. (Only linklist_add_sorted uses
the cmp function, so there are no issues about deleting via that
function - the delete functions take pointers to struct listnode or
the data itself.)

Gilad Arnold wrote:
> Hasso Tepper wrote:
> > I have no time to check at the moment (I have to run) but can you
> > look at code more? I remember that before that change new node
> > was added to the end of list, not after these elements with which
> > cmp returned 0.
>
> No, it would be added to the end of the list only if no greater
> element was found after the equal elements... (in this case,
> clearly the tail of the list and the tail of the equal elements
> sub-list are the same location ;-> )
>
> IOW, I would simply go back to the original behavior, which you
> virtually did (apart from relocation of mutual malloc/assignment
> statements, a good practice).

You are right as always :). Reverted.

--
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator