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
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 36368 Build 33257: arc lint + arc unit
Event Timeline
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 1417 | I'd reverse the logic here. If the allocation fails break out. That moves the bulk of the code one level of indentation down. | |
| 1423 | Is there a point in doing repeated allocations here? We know the maximum length of an interface name. | |
| sys/net/if.c | ||
| 2714 | break; Then the 'else' isn't needed and the bulk of the code moves one level of indentation down. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 1417 | That looks wrong. If allocation fails we do the ioctl(), otherwise we warn about an allocation failure. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 1417 | 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. | |
Comment Actions
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.
Comment Actions
Was actually a missing { (or one too many }) on the final else in the new block in ifconfig.c
Comment Actions
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.
Comment Actions
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)
Comment Actions
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).
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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? | |
Comment Actions
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.
Comment Actions
I've created D39659 as an alternative wy to get this info from the kernel. If we still want this information in the ifconfig, I'll come up with the diff soon.
Comment Actions
I do not think it is a good idea.
I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that
# ifconfig bridge0 create name br0 br0 # ifconfig bridge0 create bridge0
will be possible.
If this change is applied then you will get the same original name of two different interfaces.
BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?
case IFDATA_DRIVERNAME:
/* 20 is enough for 64bit ints */
dlen = strlen(ifp->if_dname) + 20 + 1;
if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) {
error = ENOMEM;
goto out;
}
if (ifp->if_dunit == IF_DUNIT_NONE)
strcpy(dbuf, ifp->if_dname);
else
sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit);
error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1);
if (error == 0 && req->newptr != NULL)
error = EPERM;
free(dbuf, M_TEMP);
goto out;Comment Actions
Does it refer to the first statement (kernel part) or the second part (ifconfig part) ? :-)
I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that
# ifconfig bridge0 create name br0 br0 # ifconfig bridge0 create bridge0will be possible.
If this change is applied then you will get the same original name of two different interfaces.
BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?
I'm actually going to deprecate this ( please see https://lists.freebsd.org/archives/dev-commits-src-main/2023-March/013718.html for more details).
Additionally, it provides wrong results (as noted by kp@ in D39659 for some interface cloners).
case IFDATA_DRIVERNAME: /* 20 is enough for 64bit ints */ dlen = strlen(ifp->if_dname) + 20 + 1; if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) { error = ENOMEM; goto out; } if (ifp->if_dunit == IF_DUNIT_NONE) strcpy(dbuf, ifp->if_dname); else sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit); error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1); if (error == 0 && req->newptr != NULL) error = EPERM; free(dbuf, M_TEMP); goto out;
Comment Actions
Sorry this is an excessive late response .
I think I was referring to D39689 which stores original name of interface, and mainly the kernel part.
I do not familiar with NETLINK so no option on the second part.
I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that
# ifconfig bridge0 create name br0 br0 # ifconfig bridge0 create bridge0will be possible.
If this change is applied then you will get the same original name of two different interfaces.
So we will have same original name for two different interfaces.
BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?
I'm actually going to deprecate this ( please see https://lists.freebsd.org/archives/dev-commits-src-main/2023-March/013718.html for more details).
Additionally, it provides wrong results (as noted by kp@ in D39659 for some interface cloners).case IFDATA_DRIVERNAME: /* 20 is enough for 64bit ints */ dlen = strlen(ifp->if_dname) + 20 + 1; if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) { error = ENOMEM; goto out; } if (ifp->if_dunit == IF_DUNIT_NONE) strcpy(dbuf, ifp->if_dname); else sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit); error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1); if (error == 0 && req->newptr != NULL) error = EPERM; free(dbuf, M_TEMP); goto out;
The above BTW is for this change, a new interface SIOCGIFDNAME . See also D42721 which try to utilize IFDATA_DRIVERNAME .