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

[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.flykt at 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