On Fri, Dec 13, 2024 at 03:16:03PM +0100, Theo Buehler wrote:
> On Fri, Dec 13, 2024 at 08:04:01AM +0100, Claudio Jeker wrote:
> > This diff implements rfc8538 (hopefully, it is hard to test for me).
> 
> I can't help with that either, unfortunately.
> 
> > This is an extension to graceful restart (which is already something I
> > very much dislike). I implemented the NOTIFICATION bits as specified but
> > decided to take a much more strict stand when to send a graceful reset.
> > 
> > bgpd only sends graceful notifications for a few cease cases (same as in
> > the RFC) and for the holdtimer and sendholdtimer errors. Everything else
> > is a hard error because the other side is not trustworthy.
> > 
> > The logic in change_state() for handling graceful restart is IMO not quite
> > correct. Especially the inclusion of EVNT_CON_FATAL bothers me since that
> > error is used in many places. Since this is already like this I did not
> > bother to fix this nicer. I will have to circle back to this.
> 
> fine with me to circle back.
> 
> > bgpd only implement the procedures for the Receiving Speaker and for the
> > time being I don't want to change that. I do not beleive in the concept of
> > graceful restart and think it is better to use GRACEFUL_SHUTDOWN and
> > localpref 0 to route traffic around the system before doing the restart.
> 
> This generally reads fine to me. Importantly, I can't spot a behavior
> change unless the feature is enabled.
> 
> ok tb
> 
> One nit is that you mix RFC8538 and RFC 8538, it would be nice to stick
> to one spelling.

Yes. Undecided which way I should spell RFC references. I think I should
walk over the tree and make all RFC8538 (without the space). What you
think?
 
> Another total bikeshed:
> 
> > +	/* RFC 8538 graceful notification: both sides need to agree */
> > +	p->capa.neg.grestart.grnotification =
> > +	    (p->capa.ann.grestart.grnotification &&
> > +	    p->capa.peer.grestart.grnotification) != 0;
> 
> why != 0?

Other code uses the same. I think at some point the code used a different
check there and using != 0 made it always return 0 or 1. But it is indeed
not needed. Maybe I confused && with &... Another cleanup task for later.

-- 
:wq Claudio