Adding libxo support to ifconfig (XML / JSON output)

[paige_paige.bio]

Summary

This change introduces a --libxo <fmt> option to /sbin/ifconfig which provides a means to export interface state data as JSON, XML, or HTML.

Summary of what has changed:

1. refactored printf output into individual function calls; all reside in new file: ifconfig_output.c
2. Each function in ifconfig_output.c also uses an ifdef gate to separate printf output from libxo output (libxo functionality is optional at build time)
3. existing printf related functions were also moved to ifconfig_output.c and also now have ifdef gates
4. all functions are prefixed with the name of the file that their printf statements came from:
5. ifconfig_print_metric
6. ifieee80211_print_channels
7. af_inet_print_addr_ppp
8. af_inet6_print_scopeid
9. etc
10. if_err, if_errc, if_errx, if_warn, if_warnc, if_warnx wrappers were created and all standard err/errx/warn/warnx/c calls were refactored to use the wrappers; the wrappers are gated to use either the standard calls or the libxo-provided err/warn functions.
11. in several cases, the print functions that already existed like print_media, etc referenced utility functions that were kept in their respective source files but converted to non-static; in cases where this was done a .h file was created for their prototypes and they get included by ifconfig_output.c.
12. Added a basic test that builds ifconfig with and without libxo, creates anumber of cloneable interfaces, then tests that the output of ifconfig with and without libxo are consistent.
13. Formatted ifconfig_output.c and ifconfig_output.h with clang-format; used the .clang-format file in the root of the freebsd-src tree.

Test Plan

Currently I'm building this with:

• CFLAGS="-DWITH_LIBXO" make -C sbin/ifconfig
• /usr/obj/mnt/src/amd64.amd64/sbin/ifconfig/ifconfig --libxo xml | xmllint - --pretty 1

For tests:

• make -C sbin/ifconfig/tests/ && make -C sbin/ifconfig/tests/ install
• kyua .. I forget so does my shell history

There are test cases included in this but they could be expanded a bit, would be nice to also add this to the tests:

• /usr/src/contrib/libxo/xolint/xolint.pl /usr/src/sbin/ifconfig/*.c

[paige_paige.bio]

I really just wanted to apologize in advance about the sheer size of this and I suspect it will need a few pair of eyes / a bit of testing and I've already tested a few including SFP info:

./ifconfig -vvvvv --libxo json | jq
...
        "sfp_id": "SFP/SFP+/SFP28",
        "sfp_physical_spec": "Unknown",
        "sfp_connector": "Copper pigtail",
        "sfp_vendor": "OEM",
        "sfp_part_number": "SFP-H10GB-CU0.5M",
        "sfp_serial": "210423022",
        "sfp_date": "2021-04-23",
        "sfp_dump": [
          3,
          4,
          33,
...

and I've done my best to in every case make sure that the text output is 1:1 with ifconfig pre-libxo every time that I've had to make changes, but I'd personally still like to make sure some things like CARP are correct (I don't have any CARP configurations off hand that I can test with)

EDIT:

Give me a day or so at least, and I will get a complete/official list of interfaces that are tested / confirmed to display correctly and send updates, Kurt also suggested potentially subscribing some related users (anybody who's also working on ifconfig or similar things like implementing libxo) I will try to get to this sooner rather than later. Adrian@ mentioned the desire to see some kind of regression testing for ifconfig output so that is probably something worth considering too.

As per my e-mail with Adrian he also said:

It's also an interesting deep dive into ifconfig to see what stuff in ifconfig should really be moved into libraries that do work (like what we're slowly doing with 802.11 stuff into its own library)

One that comes to mind immediately for me is the libifconfig library; I've looked at it many times and am always disappointed because short of vendoring it in your project you can't link against it because it's not installed to /usr/lib nor is there a static archive in /usr/lib; it's only linked against by ifconfig but also the scope of what it provides is rather limited. I don't know the story with it but it almost seems like an unfinished refactoring project that might have been lost to time (or maybe it does exactly what it needs to do and nothing else.) I gather you can install libifconfig from /usr/ports though, too and I'd guess maybe that has something to do with why it's not available via /usr/lib.

[paige_paige.bio]

I've found a couple of things on my own that I'd feel really dumb if I didn't speak up about now so I will just merely point out there's some really obvious formatting stuff, lack of comments on a couple of functions, I really just wanted to get some other folks looking at this before I step too deep. At this point I'm starting to weight the possibility of creating some ATF / unit tests against keeping the scope of this relatively contained. There's really a couple of things to consider about that

• There are a number of functions which start with print_ int the name that don't require a context (ctx) that also call xo_emit (easiest)
• There are also quite a few that do require a ctx because they're dealing with live interfaces, but they also in most cases substitute xo_emit for printf and it would probably be most beneficial if they were tested, but it would also in a lot of cases have to create a live interface before (harder)
• There's a lot of replacements for warn, err, etc for xo derivatives (xo_err, xo_warn etc), in a few places there are substitutions for perror commented /* perror */ that I need to fix function defined locally but asides from that mistake (which I will fix) all of the err / warn functions and errx/warnx seem to have equivalents and it's probably a waste of time to test those. (These didn't exist before for err/warn so may be not for this scope, if there's a reason to at all.)

I can't imagine anybody is really expecting tests for xo_err / xo errx / xo_warn / xo_warnx, other than the perror substitutions that I need to change back to perror (perror/perrorc just called errx/errc, so thats fine xo_errx and xo_errc.) I'm not looking forward to doing the harder ones, just because I'll have to code standing up the interface, testing the interface, tearing down the interface and its gonna be a bit but maybe it won't be so bad. I don't know what anybody is expecting but I'd personally prefer to tell you that's what I have in mind vs: bash -c 'diff -y <(/sbin/ifconfig) <(./ifconfig)' because lol I don't want to ship something that is going to break people's scripts and somebody say "oh well go figure there were no test cases." Let me see how hard it would be to come up with some ATF tests and see if we can easily address the possibility for regression, I'll follow up in a bit.

[paige_paige.bio]

fixing a couple of errors where I used xo_warn to replace calls to Perror by changing the calls back to Perror instead of xo_warn

[kp]

In D58560#1344488, @paige_paige.bio wrote:

fixing a couple of errors where I used xo_warn to replace calls to Perror by changing the calls back to Perror instead of xo_warn

Did you inadvertently revert most of the change in the latest update?

Testing the previous iteration I found it caused a number of pf tests to fail. I'm not entirely sure why. It looks as if ifconfig epair create may not return the name in the expected way, but I don't fully understand what broke that. In any event, I wanted to point at the test suite in general as a good place to start checking for inadvertent output changes.

[paige_paige.bio]

Sorry for the delayed response its been really hot here, lots of smoke in the Seattle area

Did you inadvertently revert most of the change in the latest update?

I'm really not sure what happened, I made another patch to fix something I had done wrong, I couldn't figure out how to get arcanist to make an update so I made a diff and attached it through the reviews page-- I can't delete it either I wish I had waited. Maybe at some point we could jump on a video
call or something and figure this out.

Testing the previous iteration I found it caused a number of pf tests to fail. I'm not entirely sure why. It looks as if ifconfig epair create may not return the name in the expected way, but I don't fully understand what broke that. In any event, I wanted to point at the test suite in general as a good place to start checking for inadvertent output changes.

Yeah so I noticed that, I think it's some space or something I might have to look a diff of the output with hexdump -C to find the actual difference, I will though I just wanted to say I noticed it too.

I saw somewhere, maybe on the mailing list that somebody had mentioned that ifconfig is also in rescue so I need to make sure that this change doesn't impact rescue, I'm guessing I might need to include ifdef gates for all of the print statements or maybe make some local wrapper methods then refactor this.

[paige_paige.bio]

Did you inadvertently revert most of the change in the latest update?

yeah that was definitely not the intention little bit aggravated by this. Why can't you delete the changeset?

[paige_paige.bio]

There doesn't appear to be any problems with this and rescue, I just tested it and it works fine: https://gist.github.com/paigeadelethompson/27b6f3f7c6763c44b56c4229125dfc81

I think the only thing left to do is for one figure out how to do this changeset the right way and fix some text output issues.

[paige_paige.bio]

Im also gonna have to manually fix the formatting issues because clang-format isn't gonna work for this as near as I can tell (at least not with this .clang-format file in /usr/src..) @kp I dont want to live in the comments too much so I've kinda taken to discord to try to find some help getting the diff sorted out but if I can't find the answer I'm looking for there then I'll pick through the docs a little more and eventually resort to submitting diff 3 to cancel diff 1 & 2 if I gotta and I'll be sure to get that one right. Hoping to at least have some tests up here by the end of this weekend (I'll try to anyway)

[paige_paige.bio]

Ive been thinking about another way to do this that might work better aynway, and it would also involve just adding a gate for but:

• create ifconfig_output.c / h
• create print_* functions for each printf (these would go in ifconfig_output.c)
• and then each one can have an ifdef gate where one uses the normal print statement and the other uses libxo
• this can also contain wrappers (if_warn if_error etc) and those can be gated as well.

What doesn't feel right is having something like if/else where the program is compiled with both printf and libxo because libxo is kind of designed to work almost like a drop in replacement (still have to rewrite the formatters) but this would at least provide a means to test this more reliably since getting a reliable test will depend on having something for reference (old printf ifconfig) so to get to testing this would require:

1. build ifconfig without libxo
2. build another copy without libxo
3. compare the text output of both to ensure they're the same

and then perhaps eventually the ifdef gate and old printf statements can be removed (or they can be kept for posterity, maybe it makes more sense given ifconfig exists in rescue, not sure how much the static archive for libxo adds to the footprint but it might be worth noting.

I'll investigate this a little bit more tomorrow this should be a pretty easy thing to change just tedious.

[paige_paige.bio]

I've made some progress on refactoring ifconfig so that all of the printf statements are maintained with LIBXO being optional. I'm not quite ready to push a change to arcanist yet, but I'm very close: https://github.com/paigeadelethompson/freebsd-src/commits/ifconfig_libxo_support-8-16-26/

this way if there are any concerns about libxo, then it's optional. I've moved all console/io related functions to ifconfig_output.c/h, and all output functions are prefixed with the name of the file that they came from like:

void af_link_print_pcp(if_ctx *ctx);
void ifbridge_print_vlans(ifbvlan_set_t *vlans);
void ifconfig_printifnamemaybe(void);

some print related functions already existed, a lot more were created to avoid changing how the program works (the down side might be that somebody might not like the number of print utility functions that this adds:

https://github.com/paigeadelethompson/freebsd-src/blob/ifconfig_libxo_support-8-16-26/sbin/ifconfig/ifconfig_output.h

I wish I could say I vibe coded this but no I had to do this by hand. it was a pretty miserable task and I still have a number of things I need to do before I can submit it thru arcanist but I just wanted to provide an update that I'm still working on this.

The other plus side to decoupling the printf code is that this is much easier to create unit tests for (testing the difference between the old text and libxo; which should be especially useful since the intention is to maintain both I assume. I'm not a fan of the file size of ifconfig_output but its also not the worst I've ever seen. I still need to

• double check each printf string with the old code to make 100% sure they haven't gotten mangled
• do the same for the libxo strings + check with the linter
• fix some LIBXO gating in ifconfig.c
• make sure that the NETLINK gating is correct everywhere it should be (need to test build without netlink and see if it builds and fix it if it doesn't)
• make sure the signatures for the if_err/errc/errx/warn/warnc/warnx wrappers are correct only if_err and if_errx currently have attribute((noreturn)); I think some others are supposed to as well
• After I'm through all of that, I need to squash this into a single commit and compare a diff, and fix any unnecessary changes, then I can pull from upstream, make sure everything is up to date
• Finally I can then experiment with creating some kind of test case that focuses specifically on the output of the program.

Notes:

some of the print functions referenced helper functions which don't deal with printing to console specifically and I left them in their respective files, but converted them to non-static/added a .h file where applicable that gets included in ifconfig_output.c. Hopefully soon but I just wanted to say I am still working on this a bit

EDIT:
just realized I could clean a lot of the *_print_* functions with goto / labels and it should be a clean way to keep the duplication down.

[paige_paige.bio]

This diff should take precedence over the previous diffs; the previous diffs can be deleted I have no use for them only this one matters now, still can't get arcanist to work right either (ask for screenshots if it matters.) I've updated the summary as well to describe everything that has changed (it's more or less just the same commit message I used for my local repo: https://github.com/paigeadelethompson/freebsd-src/commit/28e0d8661b7f3e5c27a46a3d439a6452329abe14