Page MenuHomeFreeBSD

ifconfig: Reuse media state in ifmedia_getstate
ClosedPublic

Authored by freqlabs on Apr 13 2021, 6:04 AM.

Details

Summary

Fixes regression after 2803fa471e77dc8f227fe00bbf075de7feb10022 where
changing media options fails.

Reported by: np
MFC after: immediately

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

I can confirm that this change fixes the problem for me.

Just waiting for approval on this.

donner requested changes to this revision.Apr 15 2021, 7:59 PM
donner added a subscriber: donner.

So there is a singleton constructor, which caches the result for later usage?
But setmedia will free the struct without resetting the singleton?

sbin/ifconfig/ifmedia.c
187

There is the free(3) call.

198

This retrieves the singleton value.

216

This callback could free the "ifrm" pointer, which is returned from the singleton call above.

This revision now requires changes to proceed.Apr 15 2021, 7:59 PM

I'm restoring behavior that was removed by mistake, so awkward as it may be, this is how ifconfig already worked before I tidied it up.

We only want to get the media status once, because we make changes to it in multiple operations. Right now we're applying each change to a fresh state, rather than accumulating the changes. When we finally get into the callbacks, we end up setting (and freeing) the result of only one operation, then the rest of the changes are skipped (because the callback is a one-shot).

sbin/ifconfig/ifmedia.c
187

The callback is a one-shot (see the next line) so we only free this once. That confused me at first, too.

We only want to get the media status once, because we make changes to it in multiple operations. Right now we're applying each change to a fresh state, rather than accumulating the changes. When we finally get into the callbacks, we end up setting (and freeing) the result of only one operation, then the rest of the changes are skipped (because the callback is a one-shot).

What a ugly piece of code.
If the ifmr would be a static global variable, the "free" could set it to NULL.

This revision is now accepted and ready to land.Apr 15 2021, 10:56 PM