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
