On Tue, Mar 22, 2022 at 11:40:12AM +0100, Theo Buehler wrote:
> On Tue, Mar 22, 2022 at 10:55:48AM +0100, Claudio Jeker wrote:
> > As mentioned I need a TAILQ for the list of prefixes that belong to a rib
> > entry. Mainly because I need TAILQ_PREV. This diff does this replacement.
> > I did not change the nexhtop LIST of prefixes to a TAILQ. Maybe something
> > to consider but there is no real need for that.
> >
> > This is mostly a mechanical change. The only thing that had to change is
> > rde_softreconfig_sync_reeval() where before the LIST_HEAD was copied which
> > is not possible with TAILQ (there is a pointer to the TAILQ_HEAD struct
> > from inside the TAILQ). Instead use TAILQ_CONCAT() to move the queue from
> > the rib_entry to the local tailq head. I checked the rest of the code and
> > did not find any other case where the rib_entry prefix_h was copied
> > around.
> >
> > I also have a fix for the unittest regress ready for this change. I left
> > that one out since it is just a trivial mechanical change.
>
> ok
>
> One question on the changes in prefix_{insert,remove}():
>
> > @@ -321,14 +321,8 @@ prefix_insert(struct prefix *new, struct
> > * MED inversion, take out prefix and
> > * put it onto redo queue.
> > */
> > - LIST_REMOVE(xp, entry.list.rib);
> > - if (tailp == NULL)
> > - LIST_INSERT_HEAD(&redo, xp,
> > - entry.list.rib);
> > - else
> > - LIST_INSERT_AFTER(tailp, xp,
> > - entry.list.rib);
> > - tailp = xp;
> > + TAILQ_REMOVE(&re->prefix_h, xp, entry.list.rib);
> > + TAILQ_INSERT_TAIL(&redo, xp, entry.list.rib);
>
> The above looks right: you keep appending at the end. In prefix_remove()
> you flipped the insertion to the start. Shouldn't the below be changed
> to use TAILQ_INSERT_TAIL() as well?
Yes, indeed. That should be a TAILQ_INSERT_TAIL().
Not sure if it matters that much but it is better to keep the order on the
redo queue to limit recursion when reinserting them at the end.
> > @@ -402,22 +396,16 @@ prefix_remove(struct prefix *old, struct
> > * possible MED inversion, take out prefix and
> > * put it onto redo queue.
> > */
> > - LIST_REMOVE(xp, entry.list.rib);
> > - if (tailp == NULL)
> > - LIST_INSERT_HEAD(&redo, xp,
> > - entry.list.rib);
> > - else
> > - LIST_INSERT_AFTER(tailp, xp,
> > - entry.list.rib);
> > - tailp = xp;
> > + TAILQ_REMOVE(&re->prefix_h, xp, entry.list.rib);
> > + TAILQ_INSERT_HEAD(&redo, xp, entry.list.rib);
>
--
:wq Claudio
