Re: [PATCH v2 2/2] procfs: Populate /proc/route with network routes

[Samuel Thibault]

Hello,

The principe looks good, just a few comments below.

Samuel

Damien Zammit, le ven. 02 sept. 2022 09:06:38 +0000, a ecrit:
> diff --git a/procfs/rootdir.c b/procfs/rootdir.c
> index 0e7c05c00..deddaa43f 100644
> --- a/procfs/rootdir.c
> +++ b/procfs/rootdir.c
> @@ -408,6 +411,70 @@ out:
>    return err;
>  }
> 
> +static error_t
> +rootdir_gc_route (void *hook, char **contents, ssize_t *contents_len)
> +{
> +  error_t err;
> +  mach_port_t pfinet;
> +  unsigned int i, len, buflen = 0;
> +  char *src, *dst;
> +  ifrtreq_t *r;
> +  char dest[16], gw[16], mask[16];

Rather use INET_ADDRSTRLEN

> +  char *inet_to_str(in_addr_t addr)
> +  {
> +    struct in_addr sin;
> +
> +    sin.s_addr = addr;
> +    return inet_ntoa(sin);

Rather use inet_ntop which is threadsafe.

> +  }
> +
> +  pfinet = file_name_lookup (_SERVERS_SOCKET "/2", O_RDONLY, 0);

Rather use AF_INET than "2".

> +  if (pfinet == MACH_PORT_NULL)
> +    {
> +      *contents_len = 0;
> +      err = errno;
> +      goto out;

Not out, since here we don't have a port to deallocate.

> +    }
> +
> +  err = pfinet_getroutes (pfinet, -1, &src, &buflen);
> +  if (err)
> +    {
> +      *contents_len = 0;
> +      goto out;
> +    }
> +
> +  r = (ifrtreq_t *)src;
> +  *contents_len = buflen / sizeof(ifrtreq_t) * 200 + 128;

200 ? 128 ?

Use macro to name them.

> +  *contents = calloc (1, *contents_len);
> +  if (! *contents)
> +    {
> +      err = ENOMEM;
> +      goto out;
> +    }
> +
> +  dst = *contents;
> +  sprintf(dst, "%-127s\n", "Iface\tDestination\tGateway\t 
> Flags\tRefCnt\tUse\tMetric\tMask\t\tMTU\tWindow\tIRTT");

Instead of %-127s you can use %-*s, to be able to use the macro.

> +  dst += 128;
> +
> +  for (i = 0; i < buflen / sizeof(ifrtreq_t); i++)
> +    {
> +      sprintf(dest, "%-15s", inet_to_str(r->rt_dest));
> +      sprintf(gw,   "%-15s", inet_to_str(r->rt_gateway));
> +      sprintf(mask, "%-15s", inet_to_str(r->rt_mask));

There as well you can use %-*s and INET_ADDRSTRLEN.

> +      len = sprintf(dst, "%s\t%s\t%s\t%04X\t%d\t%u\t%d\t%s\t%d\t%u\t%u\n",

Again, use snprintf, precisely to use the macro for 200 here.

> +                 r->ifname, dest, gw, r->rt_flags, 0, 0,
> +                 r->rt_metric, mask, r->rt_mtu, r->rt_window, r->rt_irtt);
> +      dst += len;
> +      r++;
> +    }
> +
> +out:
> +  mach_port_deallocate (mach_task_self (), pfinet);
> +  return err;
> +}
> +
>  static struct node *rootdir_self_node;
>  static struct node *rootdir_mounts_node;
>