libifconfig: First baby steps towards libification of ifconfig

[marieheleneka_gmail.com]

Summary

Libifconfig will implement most things ifconfig does today, and it is my intention to reduce complexity of future ifconfig maintenance and development by refactoring ifconfig to use it once the library stabilizes more. (Some work is done on this already)

This is an initial patch to get feedback on the first baby steps, and maybe get it into the tree, which would hopefully make it easier to experiment with and increase amount of feedback.

The API is not currently stable. It may change entirely at any time, but it's safe to experiment with.
See https://github.com/Savagedlight/libifconfig/tree/master/examples for basic usage examples.

The manpage is nonexistent at the moment, but between the examples and documentation in the header files you should find your way around.

[marieheleneka_gmail.com]

Removed things that are not related to FreeBSD base.

[gnn]

A few points about how we put code into FreeBSD.

lib/libifconfig/.gitlab-ci.yml

1 ↗

(On Diff #19341)

I don't think this file is supposed to be here?

lib/libifconfig/LICENSE.txt

1 ↗

(On Diff #19341)

We don't normally have a LICENSE file. Just put the appropriate license into the C and include files.

lib/libifconfig/examples/ifcreate.c

1 ↗

(On Diff #19341)

Please move the examples into the examples directory in share.

[kp]

Very quick first pass remarks.

The source and headers should probably be moved directly into lib/libifconfig.

Bikeshed: libifconfig or libifc?

lib/libifconfig/Makefile

5

This (and SRCS/INCS/INCSDIR) should be indented with tabs.

share/mk/bsd.libnames.mk

85

This should be indented with a tab.

[bcr]

Typo fixes in comments.

lib/libifconfig/examples/ifcreate.c

51 ↗

(On Diff #19347)

superfluous space after "do".

77 ↗

(On Diff #19347)

s/accomodating/accommodating/

lib/libifconfig/examples/ifdestroy.c

51 ↗

(On Diff #19347)

Superfluous space after "do".

lib/libifconfig/examples/setdescription.c

48 ↗

(On Diff #19347)

Superfluous space after "do".

lib/libifconfig/examples/setmtu.c

52 ↗

(On Diff #19347)

Superfluous space after "do".

[kp]

lib/libifconfig/src/libifconfig.c

135 ↗

(On Diff #19347)

This will likely be easier to read if the conditions are inverted, i.e. check for error, not success.

Something like:

if ((desc = realloc(desc, descrlen)) == NULL) {
    h->error.errtype = OTHER;
    h->error.errcode = ENOMEM;
    return (-1);
}

ifr.ifr_buffer.buffer = descr;
ifr.ifr_buffer.length = descrlen;

if (libifc_ioctlwrap(h, AF_LOCAL, SIOCGIFHESCR, &ifr) != 0)
    return (-1);

if (ifr.ifr_buffer.length > descrlen) {
    descrlen = ifr.ifr_buffer.length;
    continue;
}

...

196 ↗

(On Diff #19347)

The maximum length is configured through the net.ifdescr_maxlen sysctl.
If the name is longer the ioctl() will return ENAMETOOLONG.

I don't think you need to check for this before the ioctl() call. Just handle the error.

206 ↗

(On Diff #19347)

It's safe to call free(NULL). You don't need this check.
Also, I don't think it can be NULL at this point. You can trust the kernel to not remove the buffer for you.

244 ↗

(On Diff #19347)

The kernel doesn't truncate, and if the name is longer than it likes you'll get an error.

lib/libifconfig/src/libifconfig_internal.c

87 ↗

(On Diff #19347)

The maximum number of address families (looking at /usr/include/sys/socket.h) seems to be low enough that this shouldn't be required. AF_MAX is 42 (a pleasing number).

Just allocating a fixed array of AF_MAX will waste very little memory, simplify things, and get rid of the lookup loop in libifc_socket().

[marieheleneka_gmail.com]

Addressed feedback

[marieheleneka_gmail.com]

lib/libifconfig/src/libifconfig.c

135 ↗

(On Diff #19347)

Chop all the x-mas trees. :)

lib/libifconfig/src/libifconfig_internal.c

87 ↗

(On Diff #19347)

I agree that allocating a static amount of memory (sizeof(...)*AF_MAX) should incur an insignificant amount of wasted memory, and that the reduction in code complexity is desired.

It does however not remove the need for the lookup loop, as this is required to find the proper socket and create it if it doesn't exist.

[kp]

lib/libifconfig/src/libifconfig_internal.c

87 ↗

(On Diff #19347)

Yes, but if you allocate fds[AF_MAX] you can index based on address family and the lookup becomes a simple array access, rather than an iteration of the array.
(so fds[AF_INET], rather than a for loop).

[marieheleneka_gmail.com]

Addressed feedback.

Now assumes addressfamily will be a number less than AF_MAX and allocates an array of sockets with AF_MAX elements. All lookups and assignments of sockets are now done via array access.

[kp]

lib/libifconfig/libifconfig.c

83

You may want to initialise the sockets array elements to -1, because '0' can be a valid file descriptor.

lib/libifconfig/libifconfig_internal.c

100

So != -1 here.

107

You could assign directly to h->sockets[addressfamily] here.

[marieheleneka_gmail.com]

Addressed comments.

Also adjusted sockets array from AF_MAX to AF_MAX+1 as AF_MAX describes the highest ID of currently available AF's, and not number of available AFs. (1-based vs 0-based counting, in other words.)

[marieheleneka_gmail.com]

The only change in this revision is the library has been renamed from "libifconfig" to "libifc", to be in sync with the prefixes and what people are actually calling it. Uploading it so it will be easier to track changes between revisions here.

[kp]

share/mk/bsd.libnames.mk

85

I think you missed a rename here.

[marieheleneka_gmail.com]

Fixed a few missed renames.

[marieheleneka_gmail.com]

Removed libifc_ioctlwrap_caddrt as it was not necessary.
'struct ifreq ifr' is now always initialized by overwriting it with 0's.
Makefile now defines WARN?=6 to fault on all warnings. (Warnings are future "gifts" we don't want)

[marieheleneka_gmail.com]

Now tells lib/Makefile to build libifc, so it actually gets built with 'buildworld'
Disabled building of shared library for now, to further state this is not production ready.

[marieheleneka_gmail.com]

Added missing $FreeBSD$ tags