[systemd-devel] [RFC][PATCH] libsystemd-rtnl: add a rtnetlink library

[Lennart Poettering]

On Sun, 27.10.13 21:47, Tom Gundersen (teg at jklm.no) wrote:

> +static int message_new(sd_rtnl_message **ret, size_t initial_size) {
> +        sd_rtnl_message *m;
> +
> +        assert_return(ret, -EINVAL);
> +        assert_return(initial_size > 0, -EINVAL);

Shouldn't this check be >= sizeof(struct nlmsghdr) or so?

> +        ifi = (struct ifinfomsg *) NLMSG_DATA((*ret)->hdr);

You don't need no casts for that, as NLMSG_DATA evaluates to a void*,
and void* is implicitly upgraded to any pointer type in assignments.

> +int sd_rtnl_message_addr_new(__u16 nlmsg_type, int index, unsigned
> char family, unsigned char prefixlen, unsigned char flags, unsigned
> char scope, sd_rtnl_message **ret) {

__u16? Grrhrh! uint16_t please!

> +/* If successful the updated message will be correctly aligned, if unsuccessful the old message is
> +   untouched */
> +static int add_rtattr(sd_rtnl_message *m, unsigned short type, const void *data, size_t data_length) {
> +        __u32 rta_length, message_length;

Same here... I mean, I am pretty sure we can be sure that __u32 is the
same type as uint32_t. And if it isn't we are seriously fucked
anyway... If you want to be extra paranoid then add a
assert_cc(sizeof(uint32_t) == sizeof(__u32)) somewhere, but that's
already over the top, no...

> +static int message_receive_need(sd_rtnl *rtnl, size_t *need) {
> +        assert_return(rtnl, -EINVAL);
> +        assert_return(need, -EINVAL);
> +
> +        /* ioctl(rtnl->fd, FIONREAD, &need)
> +        Does not appear to work on netlink sockets. libnl uses
> +        MSG_PEEK instead. I don't know if that is worth the
> +        extra roundtrip.

Ah! Pity!

> +
> +        For now we simply use the maximum message size the kernel
> +        may use (NLMSG_GOODSIZE), and then realloc to the actual
> +        size after reading the message (hence avoiding huge memory
> +        usage in case many small messages are kept around) */
> +        *need = getpagesize();

Hmm, so far we used page_size() for this everywhere, which is defined in util.c

> +static int sd_rtnl_new(sd_rtnl **ret) {
> +        sd_rtnl *rtnl;
> +
> +        assert_return(ret, -EINVAL);

Just a side note: the "sd-" prefix on the file names, and the
assert_return() stuff we only did in public APIs, or in APIs that are
posed to become public soonishly. If things are not public API we
usually used assert() instead, since these things only guard against
programming errors, and for our own stuff we can be masochistic and just
fail brutally we should be nice to other users. Not saying this should
be changed, just wanted to clarify this, so that you know what you do.

Otherwise looks great!

Lennart

-- 
Lennart Poettering, Red Hat