[systemd-devel] [RFC][PATCH] networkd: add a basic network daemon

[Tom Gundersen]

On Wed, Nov 6, 2013 at 2:25 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Wed, 06.11.13 01:33, Tom Gundersen (teg at jklm.no) wrote:
>> Short-term TODO:
>>  - make rtnl calls asynchronous
>
> Don't wait for too long for this! The longer you wait the more code you
> have to rework for this. And you shouldn't underestimate how complex
> changes from sync to async are...

Yeah, it's top of the list ;-)

>> Gateway=192.168.1.1
>> Address=label at 192.168.1.23/24
>> Address=fe80::9aee:94ff:fe3f:c618/64
>
> Hmm, what's the plan regarding confguration of scopes and other
> attributes of addresses? Is the "label@" syntax your invention or has
> this been used elsewhere (I am not opposed to the syntax, just curious).

Good question. The @ syntax is my invention, but i'm very happy to
change it if anyone has a better suggestion. For the other properties
we might want, I would really like to find a syntax to get them all on
one line. I'll try figure out a more or less exhaustive list of the
properties we might want to support and suggest a syntax for it. In
the meantime I'll commit this without the "label@" support, as the
rest should be uncontroversial, and then we can add back the labeling
when we are sure it is the way we want it.

>> +        sd_event_add_io(m->event,
>> +                        udev_monitor_get_fd(m->udev_monitor),
>> +                        EPOLLIN,
>> +                        manager_dispatch_link_udev,
>> +                        (void *)m,
>> +                        &m->udev_event_source);
>
> Missing return value check! (and cast to void* is unnecessary, in C
> all pointers automatically downgrade to void* without casting anyway)
>
>> +        if (r < 0) {
>> +                log_warning("Colud not parse config file %s: %s",
>> filename, strerror(-r));
>
>                                ^^^^^^ Typo
>
>> +                return r;
>> +        } else
>> +                log_debug("Parsed configuration file %s", filename);
>> +
>> +        network->filename = strdup(filename);
>
> OOM check missing!
>
>> +static void networks_free(Manager *manager) {
>> +        Network *network, *network_next;
>> +
>> +        if (!manager)
>> +                return;
>> +
>> +        LIST_FOREACH_SAFE(networks, network, network_next, manager->networks) {
>> +                network_free(network);
>> +        }
>
> I usually just use:
>
> while ((network = manager->networks))
>         network_free(free);
>
> Which should do the job too.
>
>> +struct Link {
>> +        uint64_t ifindex;
>
> Hmm is this really an uint64_t? if_nametoindex(3) suggestes it's an "unsigned"?
>
> But yeah, looks great. Commit.

Thanks, will fix up your comments and commit.

Cheers,

Tom