/systemd

networkd / sd-netlink: add support for address label #5529

Merged
merged 3 commits into from Apr 29, 2017

Conversation

Reviewers

@torstehu torstehu

@poettering poettering

@mbiebl mbiebl

Assignees
No one assigned
Labels
good-to-merge/waiting-for-ci network
Projects
None yet
Milestone
No milestone
4 participants
@ssahani @poettering @torstehu @mbiebl
@ssahani
Contributor

ssahani commented Mar 3, 2017

IPv6 address labels are used for address selection; they are described in RFC 3484.
Precedence is managed by userspace, and only the label itself is stored in the kernel.

enp0s25.network

[Match]
Name=enp0s25

[Network]
DHCP=yes
Address = 2001:db8:f00:baa::b

[AddressLabel]
Label=199
Prefix=2001:db8:41::/64

[AddressLabel]
Label=11
Prefix=2001:db8:31::/64

[AddressLabel]
Label=123
Prefix=2001:db8:21::/64

[AddressLabel]
Label=124
Prefix=2001:db8:11::/64

[sus@maximus label]$ ip addrlabel list

prefix ::1/128 label 0
prefix ::/96 label 3
prefix ::ffff:0.0.0.0/96 label 4
prefix 2001:db8:41::/64 dev enp0s25 label 199
prefix 2001:db8:31::/64 dev enp0s25 label 11
prefix 2001:db8:21::/64 dev enp0s25 label 123
prefix 2001:db8:11::/64 dev enp0s25 label 124
prefix 2001::/32 label 6
prefix 2001:10::/28 label 7
prefix 3ffe::/16 label 12
prefix 2002::/16 label 2
prefix fec0::/10 label 11
prefix fc00::/7 label 5
prefix ::/0 label 1
src/network/networkd-address-label.c
+ }
+
+ if (k == 0xffffffffUL) {
+ log_syntax(unit, LOG_ERR, filename, line, r, "Adresss label is invalid, ignoring: %s", rvalue);
@torstehu

torstehu Mar 8, 2017

Contributor

one small typo: Adresss -> Address

@poettering poettering added the network label Mar 31, 2017

@poettering

poettering requested changes Mar 31, 2017
View changes

looks pretty OK, but a few comments

man/systemd.network.xml
+ <term><varname>Label=</varname></term>
+ <listitem>
+ <para> The label for the prefix (an unsigned integer). 0xffffffff is reserved. This
+ key is mandatory. </para>
@poettering

poettering Mar 31, 2017

Owner

please indicate the permitted range

man/systemd.network.xml
+ <varlistentry>
+ <term><varname>Prefix=</varname></term>
+ <listitem>
+ <para>IPv6 prefix separated by a <literal>/</literal> character.
@poettering

poettering Mar 31, 2017

Owner

hmm, i think this deserves a few more words, i.e. a brief explanation what an ipv6 prefix is (i.e. an address with a prefix length, separated by a slash)

src/libsystemd/sd-netlink/netlink-types.c
@@ -30,6 +30,7 @@
#include <linux/ip.h>
#include <linux/if_link.h>
#include <linux/if_tunnel.h>
+#include <linux/if_addrlabel.h>
@poettering

poettering Mar 31, 2017

Owner

hmm, please keep the includes sorted alphabetically, see CODING_STYLE

src/libsystemd/sd-netlink/netlink-util.c
+ default:
+ return false;
+ }
+}
@poettering

poettering Mar 31, 2017

Owner

why not use IN_SET() for this?

@poettering

poettering Apr 24, 2017

Owner

this can be simplified further: please write this as:

bool rtnl_message_type_is_addrlabel(uint16_t type) {
        return IN_SET(type, RTM_NEWADDRLABEL, RTM_DELADDRLABEL, RTM_GETADDRLABEL);
}

In fact, you could even move this to the header, and make it "static inline" there...

src/libsystemd/sd-netlink/rtnl-message.c
@@ -20,6 +20,7 @@
#include <netinet/in.h>
#include <stdbool.h>
#include <unistd.h>
+#include <linux/if_addrlabel.h>
@poettering

poettering Mar 31, 2017

Owner

please order includes alphabetically

src/network/networkd-address-label.c
+
+ r = sd_rtnl_message_addrlabel_set_prefixlen(req, label->prefixlen);
+ if (r < 0)
+ return log_error_errno(r, "Could not set prefixlen: %m");
@poettering

poettering Mar 31, 2017

Owner

indentation is off, multiples of 8ch please

src/network/networkd-address-label.c
+ if (r < 0) {
+ log_syntax(unit, LOG_ERR, filename, line, r, "Address label is invalid, ignoring assignment: %s", prefix);
+ return 0;
+ }
@poettering

poettering Mar 31, 2017

Owner

wouldn't it be time to add an "in_addr_prefix_from_string()" helper or so, which parses an address and its prefixlength in one go? it appears to me we have the same code for this in place at various cases...

(can happen in a later PR)

@ssahani

ssahani Mar 31, 2017

Contributor

yes will add in another PR

@poettering poettering added the reviewed/needs-rework label Mar 31, 2017

@ssahani
Contributor

ssahani commented Mar 31, 2017

updated .
@poettering

poettering requested changes Apr 24, 2017
View changes

Sorry for the late review! A couple of more notes:

man/systemd.network.xml
@@ -796,6 +796,33 @@
</variablelist>
</refsect1>
+ <refsect1>
+ <title>[AddressLabel] Section Options</title>
@poettering

poettering Apr 24, 2017

Owner

if this is an IPv6-only concept I think this should be prefixed with "IPv6", i.e. become [IPv6AddressLabel] or so in the config files.

man/systemd.network.xml
+ <term><varname>Label=</varname></term>
+ <listitem>
+ <para> The label for the prefix (an unsigned integer) in the range 0 to 65,535 or 0 to 4,294,967,295.
+ depending on 2 or 4 bytes. 0xffffffff is reserved. This key is mandatory.</para>
@poettering

poettering Apr 24, 2017

Owner

what does that mean: "depending on 2 or 4 bytes"? Can't make sense of it.

If 0xffffffff is reserved, what does that mean? If it is not an acceptable value, then you need to filter it out, and hence document that the valid range for this is 0..4294967294 instead of 0..4294967295

src/libsystemd/sd-netlink/netlink-types.c
#include <linux/if.h>
#include <linux/ip.h>
+#include <linux/if_addr.h>
+#include <linux/if_addrlabel.h>
+#include <linux/if_bridge.h>
@poettering

poettering Apr 24, 2017

Owner

why this reorder? We usually try to keep includes in alphabetical order, unless the kernel headers are borked and don't permit that

src/libsystemd/sd-netlink/netlink-util.c
+ default:
+ return false;
+ }
+}
@poettering

poettering Mar 31, 2017

Owner

why not use IN_SET() for this?

@poettering

poettering Apr 24, 2017

Owner

this can be simplified further: please write this as:

bool rtnl_message_type_is_addrlabel(uint16_t type) {
        return IN_SET(type, RTM_NEWADDRLABEL, RTM_DELADDRLABEL, RTM_GETADDRLABEL);
}

In fact, you could even move this to the header, and make it "static inline" there...

src/network/networkd-address-label.c
+ if (!addrlabel)
+ return -ENOMEM;
+
+ addrlabel->family = AF_UNSPEC;
@poettering

poettering Apr 24, 2017

Owner

isn't this an IPv6 concept? if so, why do we have a "family" field here?

src/network/networkd-address-label.c
+}
+
+
+int config_parse_address_label_prefix(const char *unit,
@poettering

poettering Apr 24, 2017

Owner

spurious double newline

src/network/networkd-address-label.c
+ if (r < 0) {
+ log_syntax(unit, LOG_ERR, filename, line, r, "Prefix length is invalid, ignoring assignment: %s", e + 1);
+ return 0;
+ }
@poettering

poettering Apr 24, 2017

Owner

I figure you should check the valid range of the prefix range here, after all it is 0..128 only iirc?

Maybe eben turn that parser into a seperate function parse_ip_prefixlen() or so, similar to parse_ip_port()?

@ssahani

ssahani Apr 25, 2017

Contributor

yes once this in I would just make a separate patch for this.

src/network/networkd-address-label.h
+
+ int family;
+ unsigned char prefixlen;
+ unsigned label;
@poettering

poettering Apr 24, 2017

Owner

hmm, the kernel expects this as uint32_t, no? I so, I think we should store it as such.

In general: I think ew should store the stuff in the type the kernel wants it as whenever we can. Often the kernel isn't entirely congruent on that, but let's still try to find the most appropriate types for things, and then stick to that across our code base.

@poettering

poettering requested changes Apr 25, 2017
View changes

There's one forgotten replacement, looks ready otherwise

man/systemd.network.xml
+ <title>[IPv6AddressLabel] Section Options</title>
+
+ <para>An <literal>[IPv6AddressLabel]</literal> section accepts the
+ following keys. Specify several <literal>[AddressLabel]</literal>
@poettering

poettering Apr 25, 2017

Owner

You forgot one [AddressLabel] → [IPv6AddressLabel] here...

src/libsystemd/sd-netlink/netlink-util.h
@@ -32,6 +32,10 @@ bool rtnl_message_type_is_addr(uint16_t type);
bool rtnl_message_type_is_route(uint16_t type);
bool rtnl_message_type_is_neigh(uint16_t type);
+static inline bool rtnl_message_type_is_addrlabel(uint16_t type) {
+ return (IN_SET(type, RTM_NEWADDRLABEL, RTM_DELADDRLABEL, RTM_GETADDRLABEL));
@poettering

poettering Apr 25, 2017

Owner

The outer () are redundant, please drop

@ssahani
Contributor

ssahani commented Apr 25, 2017

Updated.
@poettering
Owner

poettering commented Apr 25, 2017

lgtm

@poettering poettering added good-to-merge/waiting-for-ci and removed reviewed/needs-rework labels Apr 25, 2017

@ssahani
Contributor

ssahani commented Apr 26, 2017

Added address label to meson build system.
src/network/meson.build
@@ -27,6 +27,8 @@ sources = files('''
netdev/vxlan.h
netdev/geneve.c
netdev/geneve.h
+ networkd-address-label.h
+ networkd-address-label.c
networkd-address-pool.c
@mbiebl

mbiebl Apr 26, 2017

Contributor

Just a minor nitpick: @keszybz has apparently used .c before .h file consistently in all meson build files, so I would use that sorting for consistencies sake. Also, a small typo in the commit message: s/labbel/label/

@poettering

poettering Apr 29, 2017

Owner

yes, we generally try to sort the file lists in Makefile.am these days.

ssahani added some commits Apr 25, 2017

@ssahani
sd-netlink: add support for address label
30746d6
@ssahani
networkd: add support for address label 
IPv6 address labels are used for address selection; they are described in RFC 3484.
Precedence is managed by userspace, and only the label itself is stored in the kernel.

enp0s25.network

[Match]
Name=enp0s25

[Network]
DHCP=yes
Address = 2001:db8:f00:baa::b

[AddressLabel]
Label=199
Prefix=2001:db8:41::/64

[AddressLabel]
Label=11
Prefix=2001:db8:31::/64

[AddressLabel]
Label=123
Prefix=2001:db8:21::/64

[AddressLabel]
Label=124
Prefix=2001:db8:11::/64
[sus@maximus label]$ ip addrlabel list

prefix ::1/128 label 0
prefix ::/96 label 3
prefix ::ffff:0.0.0.0/96 label 4
prefix 2001:db8:41::/64 dev enp0s25 label 199
prefix 2001:db8:31::/64 dev enp0s25 label 11
prefix 2001:db8:21::/64 dev enp0s25 label 123
prefix 2001:db8:11::/64 dev enp0s25 label 124
prefix 2001::/32 label 6
prefix 2001:10::/28 label 7
prefix 3ffe::/16 label 12
prefix 2002::/16 label 2
prefix fec0::/10 label 11
prefix fc00::/7 label 5
prefix ::/0 label 1
95b74ef
@ssahani
meson: add address label to build system

Some checks haven’t completed yet

1 pending and 4 successful checks
@martinpitt
xenial-amd64 — autopkgtest running
Details
@systemd-centos-ci
default — Build finished.
Details
semaphoreci — The build passed on Semaphore.
Details
@martinpitt
xenial-i386 — autopkgtest finished (success)
Details
@martinpitt
xenial-s390x — autopkgtest finished (success)
Details
c23ae61
@ssahani
Contributor

ssahani commented Apr 26, 2017

updated . thanks !

@poettering poettering merged commit ccefd04 into systemd:master Apr 29, 2017
4 of 5 checks passed

4 of 5 checks passed

xenial-amd64 autopkgtest running
Details
default Build finished.
Details
semaphoreci The build passed on Semaphore.
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-s390x autopkgtest finished (success)
Details

@ssahani ssahani deleted the ssahani:label branch Apr 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment