Re: [Qemu-devel] [PATCH 2/2] slirp: Add classless static routes support to DHCP server

[Samuel Thibault]

Benjamin Drung, on mar. 27 févr. 2018 17:06:02 +0100, wrote:
> +    int i = 0;

Rather unsigned?
>      char *end;
> +    unsigned int route_count = 0;
>      struct slirp_config_str *config;
> +    struct StaticRoute *routes = NULL;
> +    const StringList *iter;
>  
>      if (!ipv4 && (vnetwork || vhost || vnameserver)) {
>          error_setg(errp, "IPv4 disabled but netmask/host/dns provided");
> @@ -365,6 +369,58 @@ static int net_slirp_init(NetClientState *peer, const 
> char *model,
>          return -1;
>      }
>  
> +    iter = vroutes;

Rather initialize route_count to 0 here.
> +    while (iter) {
> +        route_count++;
> +        iter = iter->next;
> +    }



> +    iter = vroutes;

Rather initialize i to 0 here.

> +    while(iter != NULL) {

> +        // Split "subnet/mask:gateway" into its components
> +        if (get_str_sep(buf2, sizeof(buf2), &gateway, ':') < 0) {
[etc.]

You should make the gateway host.s_addr by default, so the gateway or
even :gateway part can be optional.

> -    "         [,tftp=dir][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"
> +    "         
> [,route=addr/mask:gateway][,tftp=dir][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"

And document optionality in the help, of course.

> +        if (slirp->route_count > 0) {
> +            uint8_t option_length = 0;
> +            uint8_t significant_octets;
> +
> +            for (int i = 0; i < slirp->route_count; i++) {
> +                significant_octets = slirp->routes[i].mask_width / 8
> +                                     + (slirp->routes[i].mask_width % 8 > 0);

Rather use (slirp->routes[i].mask_width + 7) / 8 (and ditto below)

> +                option_length += significant_octets + 5;
> +            }
> +
> +            *q++ = RFC3442_CLASSLESS_STATIC_ROUTE;

I'd say instead of computing the option_length twice, create a variable
uint8_t *size which you make point at the size byte here, and you can
fill it after the loop.

Apart from that it looks good.

Samuel