Re: [systemd-devel] [PATCH v2 05/26] dhcp: Add option appending and parsing

2013-11-27 Thread Patrik Flykt 

Hi,

On Tue, 2013-11-26 at 00:08 +0100, Lennart Poettering wrote:
 On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
 
  +#include stdint.h
  +
  +#include protocol.h
  +
  +int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
 
  
 
 This is a memory size, right? It should be size_t rather than int, then, no?

size_t it is.

  +static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
 
 ^^ const missing?

Here the idea was to go over the whole buffer and calling the callback
with the proper code, len and value. Start of 'buf', i.e. 'code' in the
function, needs to be advanced to the next option, so its not a const
value.

  + int *message_type, dhcp_option_cb_t cb,
  + void *user_data)
  +{
  +uint8_t *code = buf;
  +uint8_t *len;
  +
  +while (buflen  0) {
  +switch (*code) {
  +case DHCP_OPTION_PAD:
  +buflen -= 1;
  +code++;
  +break;
  +
  +case DHCP_OPTION_END:
  +return 0;
  +
  +case DHCP_OPTION_MESSAGE_TYPE:
  +buflen -= 3;
  +if (buflen  0)
  +return -ENOBUFS;
  +
  +len = code + 1;
  +if (*len != 1)
  +return -EINVAL;
  +
  +if (message_type)
  +*message_type = *(len + 1);
 
 Feels weird that message_type is an int, but you fill an uint8_t into
 it, no?
 
  +if (buflen  0)
  +return -EINVAL;
  +
  +if (cb)
  +cb(*code, *len, len + 1, user_data);
 
 I'd always be careful with callback functions like this, since people
 might alter the objects you rely on while you iterate through things and
 then you are seriously confused... I'd always do iterator-like
 interfaces instead. (Doesn't matter here though, as this is an internal
 interface only...)

True. I'll take this as a warning for future code.

  +int dhcp_option_parse(DHCPMessage *message, int32_t len,
 
 Hmm, an int32_t length? Signed? And shouldn't it be size_t again?

size_t.

  +  dhcp_option_cb_t cb, void *user_data)
  +{
  +int overload = 0;
  +int message_type = -ENOMSG;
  +uint8_t *opt = (uint8_t *)(message + 1);
  +int res;
  +
  +if (message == NULL || len  0)
  +return -EINVAL;
  +
  +len -= sizeof(DHCPMessage);
  +len -= 4;
  +if (len  0)
  +return -EINVAL;
 
 Wouldn't it be better to check len = sizeof(DHCPMessage) + 4 first?
 Sounds more logical than relying on a signed size...

With the size_t changes, yes.

Cheers,

Patrik

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2 05/26] dhcp: Add option appending and parsing

2013-11-27 Thread Zbigniew Jędrzejewski-Szmek 
On Wed, Nov 27, 2013 at 04:51:42PM +0200, Patrik Flykt wrote:
 On Tue, 2013-11-26 at 00:08 +0100, Lennart Poettering wrote:
  On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
   +static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
  
  ^^ const missing?
 
 Here the idea was to go over the whole buffer and calling the callback
 with the proper code, len and value. Start of 'buf', i.e. 'code' in the
 function, needs to be advanced to the next option, so its not a const
 value.
It's the uint8_t's that are supposed to be const, not the pointer.
I.e. 'const uint8_t *buf', not 'uint8_t* const buf'.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2 05/26] dhcp: Add option appending and parsing

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

 On Tue, 2013-11-26 at 00:08 +0100, Lennart Poettering wrote:
  On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
  
   +#include stdint.h
   +
   +#include protocol.h
   +
   +int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
  
   
  
  This is a memory size, right? It should be size_t rather than int, then, no?
 
 size_t it is.
 
   +static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
  
  ^^ const missing?
 
 Here the idea was to go over the whole buffer and calling the callback
 with the proper code, len and value. Start of 'buf', i.e. 'code' in the
 function, needs to be advanced to the next option, so its not a const
 value.

The const in const uint8_t *buf declares that the buffer this points
*to* is constant, not the pointer itself. You are welcome to alter the
pointer as much as you want if you do this. Note the difference between
const uint8_t * buf and uint8_t * const buf...

The latter makes little sense, but the former makes a lot of sense.

A function declared as 

  void foo(const int a);

makes little sense, as this just means that the function internally
won't change the internal copy of a, which is pretty useless
information. 

  void foo(const int *a);

however makes a ton of sense, since it indicates that the buffer passed
in won't be changed, while the function can do whatever it likes with
the pointer copy it got internally...

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 05/26] dhcp: Add option appending and parsing

2013-11-25 Thread Lennart Poettering 
On Mon, 25.11.13 09:13, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:

 +#include stdint.h
 +
 +#include protocol.h
 +
 +int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,

 

This is a memory size, right? It should be size_t rather than int, then, no?

 +static int parse_options(uint8_t *buf, int32_t buflen, int *overload,

^^ const missing?

 + int *message_type, dhcp_option_cb_t cb,
 + void *user_data)
 +{
 +uint8_t *code = buf;
 +uint8_t *len;
 +
 +while (buflen  0) {
 +switch (*code) {
 +case DHCP_OPTION_PAD:
 +buflen -= 1;
 +code++;
 +break;
 +
 +case DHCP_OPTION_END:
 +return 0;
 +
 +case DHCP_OPTION_MESSAGE_TYPE:
 +buflen -= 3;
 +if (buflen  0)
 +return -ENOBUFS;
 +
 +len = code + 1;
 +if (*len != 1)
 +return -EINVAL;
 +
 +if (message_type)
 +*message_type = *(len + 1);

Feels weird that message_type is an int, but you fill an uint8_t into
it, no?

 +if (buflen  0)
 +return -EINVAL;
 +
 +if (cb)
 +cb(*code, *len, len + 1, user_data);

I'd always be careful with callback functions like this, since people
might alter the objects you rely on while you iterate through things and
then you are seriously confused... I'd always do iterator-like
interfaces instead. (Doesn't matter here though, as this is an internal
interface only...)

 +int dhcp_option_parse(DHCPMessage *message, int32_t len,

Hmm, an int32_t length? Signed? And shouldn't it be size_t again?

 +  dhcp_option_cb_t cb, void *user_data)
 +{
 +int overload = 0;
 +int message_type = -ENOMSG;
 +uint8_t *opt = (uint8_t *)(message + 1);
 +int res;
 +
 +if (message == NULL || len  0)
 +return -EINVAL;
 +
 +len -= sizeof(DHCPMessage);
 +len -= 4;
 +if (len  0)
 +return -EINVAL;

Wouldn't it be better to check len = sizeof(DHCPMessage) + 4 first?
Sounds more logical than relying on a signed size...

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 05/26] dhcp: Add option appending and parsing

2013-11-24 Thread Patrik Flykt 
Add functions to append and parse DHCP options. Not all options
are passed to the callback function, the ones not exposed are
pad, end, message type and overload. If indicated by the overload
option, file and sname fields will be examined for more options.

The option functions are internal to DHCP, add a new header files
for interal function prototypes.
---
v2: fix __ prefix

 src/dhcp/internal.h |   34 ++
 src/dhcp/option.c   |  181 +++
 2 files changed, 215 insertions(+)
 create mode 100644 src/dhcp/internal.h
 create mode 100644 src/dhcp/option.c

diff --git a/src/dhcp/internal.h b/src/dhcp/internal.h
new file mode 100644
index 000..328ef1c
--- /dev/null
+++ b/src/dhcp/internal.h
@@ -0,0 +1,34 @@
+/*-*- 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
+  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 http://www.gnu.org/licenses/.
+***/
+
+#include stdint.h
+
+#include protocol.h
+
+int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+   uint8_t optlen, const void *optval);
+
+typedef int (*dhcp_option_cb_t)(uint8_t code, uint8_t len, uint8_t *option,
+void *user_data);
+int dhcp_option_parse(DHCPMessage *message, int32_t len,
+  dhcp_option_cb_t cb, void *user_data);
diff --git a/src/dhcp/option.c b/src/dhcp/option.c
new file mode 100644
index 000..34aa98a
--- /dev/null
+++ b/src/dhcp/option.c
@@ -0,0 +1,181 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+  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 http://www.gnu.org/licenses/.
+***/
+
+#include stdint.h
+#include string.h
+#include errno.h
+#include stdio.h
+
+#include internal.h
+
+int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,
+   uint8_t optlen, const void *optval)
+{
+if (!buf || !buflen)
+return -EINVAL;
+
+switch (code) {
+
+case DHCP_OPTION_PAD:
+case DHCP_OPTION_END:
+if (*buflen  1)
+return -ENOBUFS;
+
+(*buf)[0] = code;
+*buf += 1;
+*buflen -= 1;
+break;
+
+default:
+if (*buflen  optlen + 2)
+return -ENOBUFS;
+
+if (!optval)
+return -EINVAL;
+
+(*buf)[0] = code;
+(*buf)[1] = optlen;
+memcpy((*buf)[2], optval, optlen);
+
+*buf += optlen + 2;
+*buflen -= (optlen + 2);
+
+break;
+}
+
+return 0;
+}
+
+static int parse_options(uint8_t *buf, int32_t buflen, int *overload,
+ int *message_type, dhcp_option_cb_t cb,
+ void *user_data)
+{
+uint8_t *code = buf;
+uint8_t *len;
+
+while (buflen  0) {
+switch (*code) {
+case DHCP_OPTION_PAD:
+buflen -= 1;
+code++;
+break;
+
+case DHCP_OPTION_END:
+return 0;
+
+case DHCP_OPTION_MESSAGE_TYPE:
+buflen -= 3;
+if (buflen  0)
+return -ENOBUFS;
+
+len = code + 1;
+if (*len != 1)
+return -EINVAL;
+
+if (message_type)
+*message_type = *(len + 1);
+
+