Page MenuHomeFreeBSD

Introduce SIOCGIFDNAME to get the interface device name
AcceptedPublic

Authored by allanjude on Jan 20 2021, 3:15 AM.

Details

Summary

Returns the if_dname and if_dunit set by if_initname()
This allows tracking the original name of the interface after it is renamed

Connect it to ifconfig, only display it if it is different from ifname

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 36380
Build 33269: arc lint + arc unit

Event Timeline

Okay from me, but let @kp and @kevans weigh in as well.

This revision is now accepted and ready to land.Jan 20 2021, 3:29 AM
sbin/ifconfig/ifconfig.c
1416

I'd reverse the logic here. If the allocation fails break out. That moves the bulk of the code one level of indentation down.

1422

Is there a point in doing repeated allocations here? We know the maximum length of an interface name.

sys/net/if.c
2713

break;

Then the 'else' isn't needed and the bulk of the code moves one level of indentation down.

Remove the loop, the buffer wont be larger than IFNAMSIZ

This revision now requires review to proceed.Jan 20 2021, 3:41 PM
sbin/ifconfig/ifconfig.c
1416

That looks wrong. If allocation fails we do the ioctl(), otherwise we warn about an allocation failure.

allanjude added inline comments.
sbin/ifconfig/ifconfig.c
1416

Good catch, while refactoring I had looked at trying to just quit early if allocation failed, to reduce another level of indent, but it didn't work out, and this didn't get switched back.

allanjude marked an inline comment as done.

Fix logic reversal caught by kp

So this doesn't actually build for me:

...
--- all_subdir_rescue ---
/usr/src/sbin/ifconfig/ifconfig.c:1373:16: error: unused variable 'ifs' [-Werror,-Wunused-variable]
        struct ifstat ifs;
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1371:18: error: unused variable 'ift' [-Werror,-Wunused-variable]
        struct ifaddrs *ift;
                        ^
/usr/src/sbin/ifconfig/ifconfig.c:1438:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        tunnel_status(s);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:16: error: a parameter list without types is only allowed in a function definition
        tunnel_status(s);
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1451:2: error: expected identifier or '('
        for (ift = ifa; ift != NULL; ift = ift->ifa_next) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1481:2: error: expected identifier or '('
        if (allfamilies)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1483:2: error: expected identifier or '('
        else if (afp->af_other_status != NULL)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:10: error: unknown type name 'ifs'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:13: error: expected ')'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                   ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:9: note: to match this '('
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
               ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1487:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFSTATUS, &ifs) == 0)
        ^
...

It looks like ifconfig may be using the installed sockio.h header rather than the one from the source repo.

In D28247#632082, @kp wrote:

So this doesn't actually build for me:

...
--- all_subdir_rescue ---
/usr/src/sbin/ifconfig/ifconfig.c:1373:16: error: unused variable 'ifs' [-Werror,-Wunused-variable]
        struct ifstat ifs;
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1371:18: error: unused variable 'ift' [-Werror,-Wunused-variable]
        struct ifaddrs *ift;
                        ^
/usr/src/sbin/ifconfig/ifconfig.c:1438:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        tunnel_status(s);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:16: error: a parameter list without types is only allowed in a function definition
        tunnel_status(s);
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1451:2: error: expected identifier or '('
        for (ift = ifa; ift != NULL; ift = ift->ifa_next) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1481:2: error: expected identifier or '('
        if (allfamilies)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1483:2: error: expected identifier or '('
        else if (afp->af_other_status != NULL)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:10: error: unknown type name 'ifs'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:13: error: expected ')'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                   ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:9: note: to match this '('
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
               ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1487:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFSTATUS, &ifs) == 0)
        ^
...

It looks like ifconfig may be using the installed sockio.h header rather than the one from the source repo.

Was actually a missing { (or one too many }) on the final else in the new block in ifconfig.c

This revision is now accepted and ready to land.Jan 21 2021, 7:07 AM

It turns out there's an edge case we didn't consider:

epair0a: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        initname: epair0
        options=8<VLAN_MTU>
        ether 02:a5:17:a0:1e:0a
        groups: epair
        media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
        status: active
        nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Also, this wants a test case or two.

In D28247#632273, @kp wrote:

It turns out there's an edge case we didn't consider:

epair0a: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        initname: epair0
        options=8<VLAN_MTU>
        ether 02:a5:17:a0:1e:0a
        groups: epair
        media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
        status: active
        nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Also, this wants a test case or two.

We could change the strncmp to use the length of 'name' instead, but what other corner cases might we run into with substrings? (vlan100 renamed to vlan1 or something)

I don't think it's a string length issue. It's an issue of incorrect assumptions. This patch assumes that the original name of the interface is always the driver name ('if_dname') plus unit number. That's not the case for epair interfaces, and I wouldn't be surprised that this also fails for vlan interfaces (where the numeric suffix is not the unit number).

In D28247#632482, @kp wrote:

I don't think it's a string length issue. It's an issue of incorrect assumptions. This patch assumes that the original name of the interface is always the driver name ('if_dname') plus unit number. That's not the case for epair interfaces, and I wouldn't be surprised that this also fails for vlan interfaces (where the numeric suffix is not the unit number).

It worked as expected when I tested creating 'bridge100' and 'vlan42'

even in the case of:
ifconfig vlan42 create vlandev igb0 vlan 42 name vlan45

but I am sure there is some driver that doesn't do this when it calls if_initname()

Right now, only epair seems to have the unexected behaviour, and the result is printing the extra line with the original interface name (I admit it isn't really the original interface name in the epair case).

Do you have a suggestion?

In D28247#632482, @kp wrote:

I don't think it's a string length issue.

I didn't really mean string length, I meant limiting the strncmp to the shorter of the two strings, so as to avoid showing the extra line in the case where the original name is a substring of the current name, but I think that assumption is too broad.

Oh. yeah, extending the ifconfig check to catch the epair case is indeed a bad idea.

The only way I can see to handle this is to allow if_epair the chance to construct the original name itself. Sadly we can't simply add SIOCGIFDNAME to epair_ioct(), that'd have been ideal. So that'd mean adding a new if_dname or whatever function pointer to struct ifnet just for this case.
Rather annoyingly the epair code doesn't even have a straightforward way of telling which one was the a interface and which one the b interface. I suppose it makes sense that it never had to care before. Shouldn't be too hard to fix either.

I think we really do want to handle epair correctly, because the primary use case I see for this is vnet related, and vnet jails very often are connected with epair interfaces.

I have never understood why we never save the original in fields and be done with it; and then we could also check the kernel so you cannot create new ones with any original or currently used name/unit; but that's even trickier; some ifnet changes might make that easier but that's probably a longer-term project.

sbin/ifconfig/ifconfig.c
1413

That's unrelated white space; just commit that ;-) Usually we add the space to the end of line but I assume it's at width=80 already?

In D28247#690380, @bz wrote:

I have never understood why we never save the original in fields and be done with it; and then we could also check the kernel so you cannot create new ones with any original or currently used name/unit; but that's even trickier; some ifnet changes might make that easier but that's probably a longer-term project.

There are some very "amusing" issues around interface naming already. Specifically there's confusion between ifnet name and ifgroup names, and the last time I looked at it the locking was either totally broken or I totally failed to understand it.
You're probably right about just storing the original name in ifnet for this, but if we want to do more than expose it to userspace the locking around it is going to hurt.