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.

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?