There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

When I was reviewing the design of CARP, I intended to introduce a new state COMPETING to resolve an annoying bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269908 .

I'm not sure it is right to do so, too little resources for CARP rather than VRRP.

I'd suggest suspend this new feature, until we have a clear state machine.

The intent is to make it a bit easier for users to figure out why a given carp address is not negotiating MASTER/BACKUP.

I think we may have some debug / trace logs for that, other than a new state SUPPRESS.

In D40317#917957, @kp wrote:

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

In D40317#918251, @melifaro wrote:

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

This one got committed.

This creates invariant (sc_suppress == 1 && sc_state == SUPPRESS || sc_suppress == 0 && sc_state != SUPPRESS). I'm not in favor of this change, but if it goes in, I would suggest to remove sc_suppress fields and use sc_state as a flag of suppressed state.