[systemd-devel] [PATCH v2 02/26] dhcp: Add DHCP client initialization

[Tom Gundersen]

On Wed, Nov 27, 2013 at 3:13 PM, Patrik Flykt
<Patrik.Flykt at linux.intel.com> wrote:
>
>         Hi,
>
> On Mon, 2013-11-25 at 23:59 +0100, Lennart Poettering wrote:
>> On Mon, 25.11.13 09:13, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:
>>
>> > +
>> > +DHCPClient *sd_dhcp_client_new(void)
>> > +{
>> > +        DHCPClient *client;
>> > +
>> > +        client = new0(DHCPClient, 1);
>> > +        if (!client)
>> > +                return NULL;
>> > +
>> > +        client->state = DHCP_STATE_INIT;
>> > +        client->index = -1;
>> > +
>> > +        client->req_opts_size = ELEMENTSOF(default_req_opts);
>> > +
>> > +        client->req_opts = memdup(default_req_opts,
>> > client->req_opts_size);
>>
>> Missing OOM check?
>
> Indeed.
>
>> > +
>> > +        return client;
>> > +}
>> > diff --git a/src/systemd/sd-dhcp-client.h b/src/systemd/sd-dhcp-client.h
>> > new file mode 100644
>> > index 0000000..65caf59
>> > --- /dev/null
>> > +++ b/src/systemd/sd-dhcp-client.h
>> > @@ -0,0 +1,33 @@
>> > +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
>> > +
>> > +#pragma once
>>
>> If this is supposed to be public API, then it should not use the "pragma
>> once" stuff. That's a gcc'ism, which is fine for internal code, but for
>> external code we try hard to stay with C89. Please use #ifdef checks for
>> the public headers hence.
>
> Missed that one. I'll add #ifdefs here.
>
>> > +#include <netinet/in.h>
>> > +
>> > +typedef struct DHCPClient DHCPClient;
>>
>> This also needs prefixing. The other public structures like this were
>> usually called in the style "sd_dhcp_client".
>
> 'typedef struct sd_dhcp_client sd_dhcp_client;' is fine here, I suppose?
>
>> > +int sd_dhcp_client_set_request_option(DHCPClient *client, const
>> > uint8_t option);
>>
>> Hmm, what's a "const uint8_t" parameter supposed to be? const only
>> really makes sense with pointer parameters, but this ins't one...
>
> Approximately a silly typo too late one evening :-)
>
>> > +int sd_dhcp_client_set_request_address(DHCPClient *client,
>> > +                                       const struct in_addr
>> > *last_address);
>>
>> struct in_addr? Didn't you plan to use simple uint32_t for this?
>>
>> Also, if this is supposed to cover ipv6 one day too, maybe call this
>> sd_dhcp_client_set_request_address4() or so?
>
> Internally an uint32_t is fine for IPv4 handling.
>
> In the public API I tried to be consistent with a future IPv6
> implementation that probably should use 'struct in6_addr' when passing
> IPv6 addresses. IPv6 could of course use some other structure as well,
> but I expected people to be most familiar with the already existing
> 'struct in6_addr'. From that it followed that using 'in_addr' in the
> public part for IPv4 would be consistent and logical. I have no problems
> using an uint32_t in the public API also if that is wanted.

Using 'struct in_addr' in the public api makes sense to me. Using an
uint32_t wouldn't be a problem, but I'm anyway just using the struct
in networkd, so I don't really see the benefit.

> I really haven't decided on function names for DHCPv6, I was thinking
> sd_dhcp6_* would be a good prefix to start with.
>
>> > +int sd_dhcp_client_set_index(DHCPClient *client, const int interface_index);
>> > +
>> > +DHCPClient *sd_dhcp_client_new(void);
>>
>> Hmm, for the public APIs we prefer returning newly allocated objects via
>> a call-by-ref pointer, and return an error code as return code. i.e.:
>>
>> int sd_dhcp_client_new(sd_dhcp_client *ret);
>>
>> or something similar. This leaves more room open to indicate more than
>> just OOM errors to the caller.
>
> Ok, I'll do that.
>
> Cheers,
>
>         Patrik
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel