Hi,
On Mon, 2013-11-25 at 23:59 +0100, Lennart Poettering wrote:
> On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
