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

[Patrik Flykt]

	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.

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