Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux: add loopback carrier for operstate up #85

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Conversation

troglobit
Copy link
Contributor

This is a proposal to fix the issue of loopback having operstate UNKNOWN, which otherwise propagates over NETCONF as well.

See separate discussion thread in Projects.

@troglobit troglobit added the enhancement New feature or request label Jul 7, 2023
@wkz
Copy link
Contributor

wkz commented Aug 6, 2023

Supplying a known operstate when we bring up lo should be sufficient to not have to carry this kernel patch.

I.e. doing the equivalent of ip link set dev lo state up accomplishes the same thing, me thinks. NOTE: This is (confusingly) not the same thing as ip link set dev lo up. The former sets IFF_UP in the device flags, while the latter sends an updated IFLA_OPERSTATE.

@troglobit
Copy link
Contributor Author

Awesome! And yeah, you're right of course, just verified locally. Unsurprisingly this command isn't documented in ip link help nor anywhere in the manpage ip-link(8), which explains why I didn't find it while looking.

So something like this, right?

diff --git a/src/confd/src/confd/ietf-interfaces.c b/src/confd/src/confd/ietf-interfaces.c
index 0cc1bff..c29e54d 100644
--- a/src/confd/src/confd/ietf-interfaces.c
+++ b/src/confd/src/confd/ietf-interfaces.c
@@ -1020,8 +1020,12 @@ static sr_error_t netdag_gen_iface(struct dagger *net,
 
        /* Bring interface back up, if enabled */
        attr = lydx_get_cattr(cif, "enabled");
-       if (!attr || !strcmp(attr, "true"))
+       if (!attr || !strcmp(attr, "true")) {
                fprintf(ip, "link set dev %s up\n", ifname);
+               /* Ensure loopback has operstate UP, not UNKNOWN */
+               if (!strcmp(ifname, "lo"))
+                       fprintf(ip, "link set dev %s state up\n", ifname);
+       }
 
        err = err ? : netdag_gen_sysctl(net, dif);

@wkz
Copy link
Contributor

wkz commented Aug 7, 2023

I think we can even get away with just this:

diff --git a/src/confd/src/confd/ietf-interfaces.c b/src/confd/src/confd/ietf-interfaces.c
index 7299ede..142ae3c 100644
--- a/src/confd/src/confd/ietf-interfaces.c
+++ b/src/confd/src/confd/ietf-interfaces.c
@@ -1021,7 +1021,7 @@ static sr_error_t netdag_gen_iface(struct dagger *net,
        /* Bring interface back up, if enabled */
        attr = lydx_get_cattr(cif, "enabled");
        if (!attr || !strcmp(attr, "true"))
-               fprintf(ip, "link set dev %s up\n", ifname);
+               fprintf(ip, "link set dev %s up state up\n", ifname);

        err = err ? : netdag_gen_sysctl(net, dif);

This would ensure that we get the same behavior for other kinds of virtual interfaces (dummy interfaces, tunnels, etc.). When tried on a physical interface, the attribute has no effect.

This ensures that the operstate is defined for all interface
types. Without supplying this, virtual interfaces (e.g. loopback,
dummies, tunnels) will report an "UNKNOWN" operstate. For ports backed
by physical (or virtualized) hardware, this option has no effect.
@troglobit
Copy link
Contributor Author

Great stuff, lgtm 👍

@troglobit troglobit merged commit 5978edc into main Aug 7, 2023
2 checks passed
@troglobit troglobit deleted the loopback-operstate branch August 7, 2023 13:50
@troglobit troglobit added this to the Infix v23.08 milestone Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants