On Wed, Nov 27, 2013 at 3:13 PM, Patrik Flykt <patrik.fl...@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.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.
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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel