Page MenuHomeFreeBSD

libifconfig: Add function to get bridge status
ClosedPublic

Authored by freqlabs on Jun 26 2020, 3:25 AM.

Details

Summary

The new function operates similarly to ifconfig_lagg_get_lagg_status and likewise is accompanied by a function to free the bridge status data structure.

I have included in this patch the relocation of some strings describing STP parameters and the PV2ID macro from ifconfig into net/if_bridgevar.h as they are useful for consumers of libifconfig.

Test Plan

I added bindings for the new functions to my Lua wrapper for libifconfig and produced the following json:

{
  "address_cache_size": 2000,
  "address_cache_lifetime": 1200,
  "forward_delay": 15,
  "hello_time": 2,
  "hold_count": 6,
  "id": "00:00:00:00:00:00",
  "max_age": 20,
  "members": {
    "lagg0": {
      "flags": [
        "LEARNING",
        "DISCOVER",
        "AUTOEDGE",
        "AUTOPTP"
      ],
      "ifmaxaddr": 0,
      "path_cost": 2000000,
      "port": 6,
      "priority": 128
    }
  },
  "priority": 32768,
  "protocol": 2,
  "root_id": "00:00:00:00:00:00",
  "root_path_cost": 0,
  "root_port": 0,
  "root_priority": 32768
}

Diff Detail

Repository
rS 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

lib/libifconfig/libifconfig_bridge.c
73 ↗(On Diff #73685)

Would it be worth doing *bridge_status = NULL here? I expect that'd reduce the likelihood of bugs in users of this function (e.g. if they fail to check the return status and try to use *bridge_status it'll be a lot more obvious).

133 ↗(On Diff #73685)

The function documentation says this does nothing if bs is NULL, but ...

lib/libifconfig/libifconfig.h
64 ↗(On Diff #73685)

(again) Sorry, any chance of having more descriptive name? (Comment would also help).

65 ↗(On Diff #73685)

As far as I understand, here we return the structure with size& list of struct ifbreq.
Any chance it can be simplified by storing the number of member interfaces in `
ifconfig_bridge_status` and pointing directly to the array of struct ifbreq*?

lib/libifconfig/libifconfig_bridge.c
46 ↗(On Diff #73685)

Any chance of having a bit more descriptive field name? status, br_status ?

92 ↗(On Diff #73685)

Maybe it's worth renaming param to something different ? :-)
It looks a bit strange when it's used everywhere except BRDGPARAM ioctl.

sys/net/if_bridgevar.h
272 ↗(On Diff #73685)

Sorry, can you please clarify what is the benefit of moving from the const array of strings containing the relevant data for the states/protos/etc to these kind of defines?

Thank you for adding the bridge support!
Please see some minor comments inline.

freqlabs edited the test plan for this revision. (Show Details)

Address feedback

freqlabs added inline comments.
sys/net/if_bridgevar.h
272 ↗(On Diff #73685)

I'm moving the useful information from the source for ifconfig into a header where it can be reused by other code. This same convention is used for example to define CARP_STATES and similarly LAGG_PROTOS already.

Thanks for making the changes!
LGTM.

lib/libifconfig/libifconfig.h
64 ↗(On Diff #73811)

(nit) members_count to follow the pattern of object_property used in 2 variables names below?

This revision is now accepted and ready to land.Jun 28 2020, 3:57 PM

Rename num_members to members_count for consistency.

Thanks for putting a fresh set of eyes on this :)

This revision now requires review to proceed.Jun 28 2020, 5:10 PM
This revision is now accepted and ready to land.Jun 28 2020, 5:26 PM
This revision was automatically updated to reflect the committed changes.