On Wed, 13.11.13 23:22, Patrik Flykt (patrik.fl...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel