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

[Lennart Poettering]

On Wed, 13.11.13 23:22, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:

> +static 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,
> +};

Shouldn't this array be const?

> +
> +int dhcp_client_set_request_option(DHCPClient *client, uint8_t option)
> +{
> +        int i;
> +
> +        if (!client)
> +                return -EINVAL;

Using assert_return() for this makes these simpler and more redable (and
also uses the _unlikely_() stuff). Use assert_return() for all those
cases where the caller made an obvious programming mistake.

Actually, to give some insight on the way how we did things so far in systemd:

- If something is internal code, then it uses assert() for checking for
  programming errors
- If something is public API code, then it uses assert_return() for
  checking for programming errors. Public APIs have the "sd_" prefix in
  their API calls.

Basically, we are more forgiving towards public users of our code than
to ourselves.

Of course, please never use assert() nor assert_return() for checking
for runtime errors as opposed to programming errors.

> +        client->req_opts_size++;
> +        client->req_opts = realloc(client->req_opts, client->req_opts_size);
> +        if (!client->req_opts) {
> +                client->req_opts_size = 0;
> +                return -ENOBUFS;
> +        }

GREEDY_REALLOC() is cool.

> +
> +        client->req_opts[client->req_opts_size - 1] = option;
> +
> +        return 0;
> +}
> +
> +int dhcp_client_set_request_address(DHCPClient *client,
> +                struct in_addr *last_addr)

The parameter should be const, no?

> +{
> +        if (!client)
> +                return -EINVAL;
> +
> +        if (client->state != DHCP_STATE_INIT)
> +                return -EBUSY;
> +
> +        client->last_addr = malloc(sizeof(struct in_addr));
> +        if (!client->last_addr)
> +                return -ENOBUFS;

Hmm, "struct in_addr" contains nothing but a uint32_t. It really sounds
overkill allocating such a structure individually on the
stack. Shouldn't this structure be directly inside DHCPClient? Or
actually, do we really want to use struct in_addr at all? It sounds so
useless, a "uint_32_t" sounds so much simpler, and easier to use...

> +
> +        memcpy(&client->last_addr, last_addr, sizeof(struct
> in_addr));

memdup() and newdup() are cool...

> +struct DHCPClient;
> +typedef struct DHCPClient DHCPClient;

The struct line is implied by the typedef line, you can remove it.
> +
> +int dhcp_client_set_request_option(DHCPClient *client, uint8_t option);
> +int dhcp_client_set_request_address(DHCPClient *client,
> +                                    struct in_addr *last_address);
> +int dhcp_client_set_index(DHCPClient *client, int interface_index);
> +
> +DHCPClient *dhcp_client_new(void);

If this is public API these calls should have a "sd_" prefix...

Lennart

-- 
Lennart Poettering, Red Hat