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

2013-11-27 Thread Lennart Poettering 
On Wed, 27.11.13 16:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:

> > > +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.

OK, sticking to libc definitions and keeping this in sync certainly
makes some sense.

> I really haven't decided on function names for DHCPv6, I was thinking
> sd_dhcp6_* would be a good prefix to start with.

Makes sense.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-11-27 Thread Tom Gundersen 
On Wed, Nov 27, 2013 at 3:13 PM, Patrik Flykt
 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 000..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 
>> > +
>> > +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

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

2013-11-27 Thread 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.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 000..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 
> > +
> > +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

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

2013-11-25 Thread Lennart Poettering 
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?

> +
> +return client;
> +}
> diff --git a/src/systemd/sd-dhcp-client.h b/src/systemd/sd-dhcp-client.h
> new file mode 100644
> index 000..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.

> +#include 
> +
> +typedef struct DHCPClient DHCPClient;

This also needs prefixing. The other public structures like this were
usually called in the style "sd_dhcp_client".

> +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...

> +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?

> +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.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-11-24 Thread Patrik Flykt 
Provide functionality for initializing a DHCP client struct, setting
interface index, last used address and additional options to request.
On initialization the most useful options are added by default.
---
v2: - public api with sd_ prefix into src/systemd/sd-dhcp-client.h
- use assert_return when checking function arguments
- use GREEDY_ALLOC, ELEMENTSOF
- use uint32_t internally for IPv4 addresses
- use in_addr in the external API for IPv4 addresses to be consistent
  with later DHCPv6 implementation

 src/dhcp/client.c|  121 ++
 src/systemd/sd-dhcp-client.h |   33 
 2 files changed, 154 insertions(+)
 create mode 100644 src/dhcp/client.c
 create mode 100644 src/systemd/sd-dhcp-client.h

diff --git a/src/dhcp/client.c b/src/dhcp/client.c
new file mode 100644
index 000..6e34ba9
--- /dev/null
+++ b/src/dhcp/client.c
@@ -0,0 +1,121 @@
+/***
+  This file is part of systemd.
+
+  Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see .
+***/
+
+#include 
+#include 
+#include 
+#include 
+
+#include "util.h"
+#include "list.h"
+
+#include "protocol.h"
+#include "sd-dhcp-client.h"
+
+struct DHCPClient {
+DHCPState state;
+int index;
+uint8_t *req_opts;
+size_t req_opts_size;
+uint32_t last_addr;
+};
+
+static const uint8_t default_req_opts[] = {
+DHCP_OPTION_SUBNET_MASK,
+DHCP_OPTION_ROUTER,
+DHCP_OPTION_HOST_NAME,
+DHCP_OPTION_DOMAIN_NAME,
+DHCP_OPTION_DOMAIN_NAME_SERVER,
+DHCP_OPTION_NTP_SERVER,
+};
+
+int sd_dhcp_client_set_request_option(DHCPClient *client, const uint8_t option)
+{
+size_t i;
+
+assert_return(client, -EINVAL);
+assert_return (client->state == DHCP_STATE_INIT, -EBUSY);
+
+switch(option) {
+case DHCP_OPTION_PAD:
+case DHCP_OPTION_OVERLOAD:
+case DHCP_OPTION_MESSAGE_TYPE:
+case DHCP_OPTION_PARAMETER_REQUEST_LIST:
+case DHCP_OPTION_END:
+return -EINVAL;
+
+default:
+break;
+}
+
+for (i = 0; i < client->req_opts_size; i++)
+if (client->req_opts[i] == option)
+return -EEXIST;
+
+if (!GREEDY_REALLOC(client->req_opts, client->req_opts_size,
+client->req_opts_size + 1))
+return -ENOMEM;
+
+client->req_opts[client->req_opts_size - 1] = option;
+
+return 0;
+}
+
+int sd_dhcp_client_set_request_address(DHCPClient *client,
+   const struct in_addr *last_addr)
+{
+assert_return(client, -EINVAL);
+assert_return(client->state == DHCP_STATE_INIT, -EBUSY);
+
+if (last_addr)
+client->last_addr = last_addr->s_addr;
+else
+client->last_addr = INADDR_ANY;
+
+return 0;
+}
+
+int sd_dhcp_client_set_index(DHCPClient *client, const int interface_index)
+{
+assert_return(client, -EINVAL);
+assert_return(client->state == DHCP_STATE_INIT, -EBUSY);
+assert_return(interface_index >= -1, -EINVAL);
+
+client->index = interface_index;
+
+return 0;
+}
+
+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);
+
+return client;
+}
diff --git a/src/systemd/sd-dhcp-client.h b/src/systemd/sd-dhcp-client.h
new file mode 100644
index 000..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
+
+/***
+  This file is part of systemd.
+
+  Copyright (C) 2013 Intel Corporation. All rights reserved.
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but