ifconfig: Use the interface name we get back when configuring if_bridge with additional operating parameters

[zlei]

Summary

For clone creating or renaming operation, the interface name get back can be different.

PR: 270618
MFC after: 3 days

Test Plan

# ifconfig epair create
epair0a
# ifconfig bridge create addm epair0a
bridge0
# ifconfig bridge0 name br0 deletem epair0a
br0

[jlduran_gmail.com]

Tangential question: Should we add some atf-sh tests with the test plan, even if simplistic, for this and some of your previous ifconfig(8) improvements? If you think it's worth the trouble, I'll try to submit them.

[zlei]

In D39396#896780, @jlduran_gmail.com wrote:

Tangential question: Should we add some atf-sh tests with the test plan, even if simplistic, for this and some of your previous ifconfig(8) improvements? If you think it's worth the trouble, I'll try to submit them.

I do not have strong options for that.
I see this *bug* was introduced by 31997bf22302c, in year 2005. I doubt it deserves a test as it seems nobody blames for about 18 years.
That is a combination of clone create without interface unit number and addm subcommands. There're also a lot of other combinations, I do not think it is practical to write tests for all of them. But I do support to write tests for basic flows.

some of your previous ifconfig(8) improvements

It may not worth a test if you meant D39282. For D39282 the kernel still check VID passed into, and it is mainly to provide a good human-readable error as @melifaro commented.

IMO, the intent here is that we should provide a good human-readable error for the innocent mistakes.
The kernel will still reject the invalid output.

@kp What do you think?

[kp]

In D39396#896877, @zlei wrote:

In D39396#896780, @jlduran_gmail.com wrote:

some of your previous ifconfig(8) improvements

It may not worth a test if you meant D39282. For D39282 the kernel still check VID passed into, and it is mainly to provide a good human-readable error as @melifaro commented.

IMO, the intent here is that we should provide a good human-readable error for the innocent mistakes.
The kernel will still reject the invalid output.

@kp What do you think?

I tend to think it's always worth having a test. It's surprising just how often even trivial tests find real bugs. Having the tests also makes future refactoring that much easier. The tests currently don't take all that much time, nor will a test case for this add a lot of runtime. (And if we get to the point where we have so many tests their run time becomes an issue I shall be glad to have that problem.)

[kp]

I'm not quite sure why ifr.ifr_name doesn't work here though. I'd expect ifclonecreate() to create the bridge, and the clone call there should have updated the name in ifr.ifr_name. After all, we copy ifr.ifr_name to name there. What am I missing?

[melifaro]

In D39396#896914, @kp wrote:

In D39396#896877, @zlei wrote:

In D39396#896780, @jlduran_gmail.com wrote:

some of your previous ifconfig(8) improvements

It may not worth a test if you meant D39282. For D39282 the kernel still check VID passed into, and it is mainly to provide a good human-readable error as @melifaro commented.

IMO, the intent here is that we should provide a good human-readable error for the innocent mistakes.
The kernel will still reject the invalid output.

@kp What do you think?

I tend to think it's always worth having a test. It's surprising just how often even trivial tests find real bugs. Having the tests also makes future refactoring that much easier. The tests currently don't take all that much time, nor will a test case for this add a lot of runtime. (And if we get to the point where we have so many tests their run time becomes an issue I shall be glad to have that problem.)

I couldn't agree more. Recently I found&fixed the old bug triggered by the netlink code which manifested from a simple if_stf test.
The only note is that I'd really love to have such tests written in python instead of using shell-scripting :-))

[zlei]

In D39396#896915, @kp wrote:

I'm not quite sure why ifr.ifr_name doesn't work here though. I'd expect ifclonecreate() to create the bridge, and the clone call there should have updated the name in ifr.ifr_name.

lldb shows that ifr.ifr_name (this is the global one) is not updated on ifclonecreate().

After all, we copy ifr.ifr_name to name there. What am I missing?

That ifr is a local var.

static void
ifclonecreate(int s, void *arg)
{
        struct ifreq ifr;
        struct clone_defcb *dcp;

        memset(&ifr, 0, sizeof(ifr));
        (void) strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));

       ...

        /*
         * If we get a different name back than we put in, update record and
         * indicate it should be printed later.
         */
        if (strncmp(name, ifr.ifr_name, sizeof(name)) != 0) {
                strlcpy(name, ifr.ifr_name, sizeof(name));
                printifname = 1;
        }
}

[kp]

In D39396#897063, @zlei wrote:

In D39396#896915, @kp wrote:

After all, we copy ifr.ifr_name to name there. What am I missing?

That ifr is a local var.

Ahh, yes, that'd do it.

One of these days someone very, very brave (braver than me, is what I'm hinting at here) is going to have to clean up ifconfig.