Page MenuHomeFreeBSD

libifconfig: Add function to get bridge status
ClosedPublic

Authored by freqlabs on Fri, Jun 26, 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

freqlabs created this revision.Fri, Jun 26, 3:25 AM
freqlabs requested review of this revision.Fri, Jun 26, 3:25 AM
kp added inline comments.Sun, Jun 28, 10:11 AM
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 ...

melifaro added inline comments.Sun, Jun 28, 11:54 AM
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 updated this revision to Diff 73811.Sun, Jun 28, 3:40 PM
freqlabs edited the test plan for this revision. (Show Details)

Address feedback

freqlabs marked 6 inline comments as done.Sun, Jun 28, 3:41 PM
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.

melifaro accepted this revision.Sun, Jun 28, 3:57 PM

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.Sun, Jun 28, 3:57 PM
freqlabs updated this revision to Diff 73813.Sun, Jun 28, 5:10 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.Sun, Jun 28, 5:10 PM
freqlabs marked an inline comment as done.Sun, Jun 28, 5:10 PM
melifaro accepted this revision.Sun, Jun 28, 5:26 PM
This revision is now accepted and ready to land.Sun, Jun 28, 5:26 PM
kp accepted this revision.Mon, Jun 29, 9:52 AM
mmacy accepted this revision.Tue, Jun 30, 11:37 PM
This revision was automatically updated to reflect the committed changes.