savagedlight / libifconfig Goto Github PK

sbin/ifconfig is organized into a base/mixin model. Some basic functionality like setting an interface description is contained in ifconfig.c, but most functionality is contained in mixin files for each feature domain. Cramming all of ifconfig's features into a single file would make for an unmanageably large file, so I suggest that libifconfig organize itself the same way. Doing so will also make the conversion of ifconfig easier. Except for a few core functions, I suggest that most of libifconfig's public functions be named ifconfig_<DOMAIN>_<GET|SET>_<THING>, for example ifconfig_group_get_groups.

Due to a copy/paste error, ifconfig_set_metric actually sets the mtu, not the metric

Although it was fine to fall-through to the later break statement in the original ifconfig code, this is not sufficient for the library. Make it an error condition and do proper error reporting.

Reported by: Ravi Pokala

Currently, when building libifconfig as part of the FreeBSD base system, the 'libifconfig' header subdirectory isn't created, and the header file(s) are not installed.

Manual workaround: Create directory /usr/include/libifconfig before installing.

Allow deletion of clonable interfaces. Look at ifconfig's ifclone.c, DECL_CMD_FUNC(clone_destroy).

Suggested pattern: int libifc_destroy(libifc_handle_t *h, const char *name);

Add API methods for removal of primary and secondary IPv4 and IPv6 addresses. See ifconfig's keyword "-alias".

Allow creation of clonable interfaces, such as vlan, tap, etc.

Suggested pattern: int libifc_create(libifc_handle_t *h, const char *name, char **ifname);

Look at ifconfig's ifclone.c, around line 120. The interesting code is in ifclonecreate(...) and its support functions.

Check whether all compilers in base supports the '#pragma once' guard lock. Do also consider corner cases, like multiple copies of the header being included, from different paths.

Reported by: Ravi Pokala

Doxygen supports XMLDoc, JavaDoc and several other documentation styles. JavaDoc style has comments that comply with style(9), and is therefore preferred over XMLDoc which uses "///" for its documentation comments, which is not ANSI-C compatible. JavaDoc is also familiar to anyone who has used tools like PHPStorm for webdev in the past.

Add support for manipulating virtual interface 'lagg'.

The libifconfig_ prefix takes up too much horizontal space, and it's clear enough to use libifc_ instead.

Currently, the error feedback is done through a global variable. This is very hostile towards multi-threading.

Instead, create a struct which keeps state for the current thread. This struct should also contain the socket cache. Applications would have to pass an instance of this struct to the API by reference.

This makes it easier to keep track of state, and applications can use libifconfig in a multithreaded way even though libifconfig is not aware of threading, so long as they use a separate instance of the state struct per thread.

Idea by: Adrian Chadd

The type should match the type of the ioctl() argument, which is unsigned long.

Reported by: Ravi Pokala

Add support for manipulating virtual interface 'bridge'.

• create interface
• add member interface (addm)
• remove member interface (deletem)
• manipulation of various properties, see ifconfig's ifbridge.c for more details.
  • flags: see do_bridgeflag(...)
  • also see do_cmd(...) and its uses

The ifconfig source has received a semantic update to the license headers of ifconfig, making the 3-clause licenses third point be numbered 3 and not 4. This should allow libifconfig to consolidate licenses.

While here, update copyright year.

Hi there,

Not a major one, but I have noticed that some changes have been done based on style(9).

You may want to change
// comment_here
style comments to
/* comment_here */
as suggested by style(9), which is also ANSI C compliant.

Regards...

The vxlan driver requires special configuration and therefore needs separate creation/initialization code.
See ifconfig: ifvxlan.c: clone_setdefcallback("vxlan", vxlan_create);

Once system integration tests (#14) are in place, it'll be possible to automatically detect memory leaks using something like Valgrind.

Investigate if the following section of code is necessary:

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

I think this tells the next ioctl() call to allocate a larger buffer for the name, but this needs digging into how the SIOCGIFDESCR ioctl() call is implemented.

ifconfig_ioctlwrap conveniently opens the right socket type and sets all of the error fields, but it only works for ioctls where the data field has type struct ifreq. I'm finding that the majority of ioctls I need to use don't use that data type. Would you agree to change ifconfig_ioctlwrap's fourth argument to an opaque void*?

There is a possible memory leak in ifconfig_get_description(). If the ifconfig_ioctlwrap() call fails the memory allocated by reallocf() is leaked.

Make a script to copy things from libifconfig git repo to the right places in the FreeBSD SRC tree, and ignore irrelevant files.

Copy Makefile.base to /lib/libifconfig/Makefile
Copy src/ to /lib/libifconfig
Copy examples/ to /share/examples/libifconfig/.

Remember to make a makefile for FreeBSD share directory, and copy only that one, not the project-specific one, for the sake of tidiness.

The vxlan driver requires special configuration and therefore needs separate creation/initialization code.
See ifconfig: ifieee80211.c: clone_setdefcallback("wlan", wlan_create);

Currently, style checks are not run when the CI fails on building the library. This is annoying to some highly caffeinated developers.

Additionally, as the style(9) checks only require the source code to be run, they shouldn't require a successful build.

/sbin/ifconfig code sometimes casts ifreq* to caddr_t, and sometimes not. Find out if there is a reason for this, or if it's an is an artifact of decades of incremental changes.

Do as appropriate with internal functions libifc_ioctlwrap() and libifc_ioctlwrap_caddr().

FreeBSD's caddr_t is defined in sys/sys/types.h as follows:
typedef char * caddr_t; /* core address */

Implement API methods to allow setting interface address and subnet mask/prefix, as done by ifconfig's "inet + netmask" and "inet6 + prefixlen" keywords.

Makefile.standalone currently has enough information to properly build the library, but not to install files where they need to go.

People (me included) are calling this 'libifc' now. Better rename files and things before doing so becomes annoying.

Create integration tests for the various parts of the API. Current examples code is a good starting point, but needs to be expanded. Look into what framework the FreeBSD project use for system integration tests, if any, before considering MacGyvering something.

The documentation of the methods are severely lacking. There are some examples in the corresponding directory, but should at least provide some basic information on what each method does.

It might be premature to write full-fledged usage examples at this point, however.

libnv reduces or removes the compatibility issue with changes in structs between versions, moving the backwards(and for applications, forwards compatibility) concerns related to that to the library, leaving less of a headache for applications using libifconfig.

It currently looks like it won't be necessary to use, but keep an eye on it as more advanced features are implemented in libifconfig.

Add support for manipulating virtual interface 'vlan'.

• assign parent interface
• assign VLAN ID
• Other? Check ifconfig's ifvlan.c

Consider adding a method for creating VLAN interface and assigning parent interface + VLAN id in one go.

Go through everything(tm) and ensure the library handles failing strdup() calls properly. It should set error state and return a value indicating error if the strdup() failure is fatal.

The ioctl() command always returns 0 or -1 (success and failure respectively), and the actual error is stored in the thread-safe global errno variable. Remove ioctl_err and have applications use the global 'errno' variable to do their further checking.