[systemd-devel] [RFC][PATCH] udev: add network link configuration tool

[Lennart Poettering]

On Sat, 26.10.13 16:16, Tom Gundersen (teg at jklm.no) wrote:

> This tool applies hardware specific settings to network devices before they
> are announced via libudev.
> 
> Settings that will probably eventually be supported are MTU, Speed,
> DuplexMode, WakeOnLan, MACAddress, MACAddressPolicy (e.g., 'hardware',
> 'synthetic' or 'random'), Name and NamePolicy (replacing our current
> interface naming logic). This patch only introduces support for
> Description, as a proof of concept. I'll post follow-up patches once we
> agree that this is the way to go.
> 
> Some of these settings may later be overriden by a network management
> daemon/script. However, these tools should always listen and wait on libudev
> before touching a device (listening on netlink is not enough). This is no
> different from how things used to be, as we always supported changing the
> network interface name from udev rules, which does not work if someone
> has already started using it.
> 
> The tool is configured by .link files in /etc/net/links/ (with the usual
> overriding logic in /run and /lib). The first (in lexicographical order)
> matching .link file is applied to a given device, and all others are
> ignored.

So, humm, I have been thinking about the place to store these
files. It's currently a bit chaotic how we distribute files in /etc. So,
looking at this I am tempted that the general rule for us how to name
things should be something like this: a) if in doubt, prefix it with
"systemd-" or place it in some "systemd" directory. b) if positively
sure that something is a file that might be
shared/read/written/implemented by other software, where the spec of the
file format is probably something to work with other projects on, then
don't prefix, don't include the "systemd" string in the name. c) for
historic reasons some exceptions apply.

/etc/hostname, /etc/locale.conf and things like that I believe fall into
b), systemd unit files into a), and udev rules into c) (though they are
candidates for a).

Now, if we follow these rules I am pretty sure that a) applies for the
networking stuff too, no?

So, maybe have /etc/systemd/network/* or so, and then stick link,
network and netdev files in there?

And then, later, when other network related bits have to be configured
somewhere, we'd follow the same logic, possibly adding more directories
beneath /etc/systemd?

> +RUN{builtin}="net_link"

Maybe call this "net_setup_link"? Otherwise "net_link" sounds so much
like "netlink"? 

> +struct link_config_ctx {
> +        LIST_HEAD(link_config, links);
> +
> +        char **link_dirs;
> +        usec_t *link_dirs_ts_usec;
> +};

Maybe define a local _cleanup_ macro here?

_cleanup_(link_configs_freep)? 

> +
> +int link_config_ctx_new(link_config_ctx **ret) {
> +        link_config_ctx *ctx;
> +
> +        if (!ret)
> +                return -EINVAL;
> +
> +        ctx = new0(link_config_ctx, 1);
> +        if (!ctx)
> +                return -ENOMEM;
> +
> +        LIST_HEAD_INIT(ctx->links);
> +
> +        ctx->link_dirs = strv_new("/etc/net/links",
> +                                  "/run/net/links",
> +                                  "/usr/lib/net/links",
> +                                  NULL);
> +        if (!ctx->link_dirs) {
> +                log_error("failed to build link config directory array");
> +                link_config_ctx_free(ctx);
> +                return -ENOMEM;
> +        }
> +        if (!path_strv_canonicalize_uniq(ctx->link_dirs)) {
> +                log_error("failed to canonicalize link config directories\n");
> +                link_config_ctx_free(ctx);
> +                return -ENOMEM;
> +        }
> +
> +        ctx->link_dirs_ts_usec = calloc(strv_length(ctx->link_dirs),
> sizeof(usec_t));

new(usec_t, strv_length(...)) sounds like the better typesafe option, no?

Isn't it sufficient to merge these timestamps into a single one that is
the newest of all timestamps you find? After all you need to reload
*all* dirs anyway if one of them changes, to get the overriding logic
right? Or am I missing something?

(I mean, I am not particularly fond of having multiple arrays that have
the same keys and life-times that simply contain different fields of the
same object. To me, it usually appears nicer to have a single array of
structs instead. That said, if you just collapse all timestamps into one
you neatly avoid the complexity for that... ;-)


> +static bool match_config(link_config *match, struct udev_device *device) {
> +        const char *property;
> +
> +        if (match->match_mac) {
> +                property = udev_device_get_sysattr_value(device, "address");
> +                if (!property || !streq(match->match_mac, property))
> {

You could use streq_ptr() here and avoid the extra check... That said, I
wonder if it wouldn't be a better idea to do a case independent compare?

Or actually, so far we actually parsed all data we could into their
binary counterparts where that's obvious and doable, to ensure that the
data passed is actually validated as a side-effect. In this case that
would probably mean storing the MAC address in an array?

> +static int builtin_net_link(struct udev_device *dev, int argc, char **argv, bool test) {
> +        link_config *link;
> +        int r;
> +
> +	if (argc > 1) {
> +		log_error("This program takes no arguments.");
> +		return EXIT_FAILURE;
> +	}

Indenting weirdness...

Otherwise looks fine!

Lennart

-- 
Lennart Poettering, Red Hat