netdev
[Top] [All Lists]

Re: [PATCH] Prefix List patch against 2.5.70

To: krkumar@xxxxxxxxxx
Subject: Re: [PATCH] Prefix List patch against 2.5.70
From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@xxxxxxxxxxxxxx>
Date: Sat, 31 May 2003 11:02:49 +0900 (JST)
Cc: davem@xxxxxxxxxx, kuznet@xxxxxxxxxxxxx, yoshfuji@xxxxxxxxxxxxxx, netdev@xxxxxxxxxxx, linux-net@xxxxxxxxxxxxxxx
In-reply-to: <3ED80230.2030508@xxxxxxxxxx>
Organization: USAGI Project
References: <3ED80230.2030508@xxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
In article <3ED80230.2030508@xxxxxxxxxx> (at Fri, 30 May 2003 18:15:28 -0700), 
Krishna Kumar <krkumar@xxxxxxxxxx> says:

> +/* prefix list returned to user space in this structure */
> +struct plist_user_info {
          ^ip6 or ipv6 or so.
> +     char name[IFNAMSIZ];            /* interface name */
        ~~~~~~~~~~~~~~~~~~~duplicate information.
> +     int ifindex;                    /* interface index */
> +     int nprefixes;                  /* number of elements in 'prefix' */
> +     struct var_plist_user_info {    /* multiple elements */
> +             char flags[3];          /* router advertised flags */
                     ~~~~~~~~this is not good interface.
> +             int plen;               /* prefix length */
> +             __u32 valid;            /* valid lifetime */
> +             struct in6_addr ra_addr;/* advertising router */
> +             struct in6_addr prefix; /* prefix */
> +     } plist_vars[0];
> +};
> +
>   extern void                 addrconf_init(void);
>   extern void                 addrconf_cleanup(void);
> 

:

I think we should use 1 fixed-length message per prefix,
instead of variable length message.


> +                     pinfo->plist_vars[count].plen = p_el->pinfo.prefix_len;
> +                     pinfo->plist_vars[count].valid = p_el->pinfo.valid -
> +                             (jiffies - p_el->timestamp)/HZ;
> +                     if ((p_el->ra_flags & (ND_RA_FLAG_MANAGED |
> +                         ND_RA_FLAG_OTHER))
> +                         == (ND_RA_FLAG_MANAGED|ND_RA_FLAG_OTHER))
> +                             strcpy(pinfo->plist_vars[count].flags, "MO");
> +                     else if (p_el->ra_flags & ND_RA_FLAG_MANAGED)
> +                             strcpy(pinfo->plist_vars[count].flags, "M");
> +                     else if (p_el->ra_flags & ND_RA_FLAG_OTHER)
> +                             strcpy(pinfo->plist_vars[count].flags, "O");
> +                     else
> +                             strcpy(pinfo->plist_vars[count].flags, "-");
> +                     ipv6_addr_copy(&pinfo->plist_vars[count].ra_addr,
> +                         &p_el->ra_addr);
> +                     for (i = 0; i < 8; i++)
> +                             pinfo->plist_vars[count].ra_addr.s6_addr16[i] =
> +                                 
> __constant_ntohs(pinfo->plist_vars[count].ra_addr.s6_addr16[i]);
> +                     ipv6_addr_copy(&pinfo->plist_vars[count].prefix,
> +                         &p_el->pinfo.prefix);
> +                     for (i = 0; i < p_el->pinfo.prefix_len/16; i++)
> +                             pinfo->plist_vars[count].prefix.s6_addr16[i] =
> +                                 
> __constant_ntohs(pinfo->plist_vars[count].prefix.s6_addr16[i]);

Absoletely nasty.
- don't use charaters to represent flags; use real flags.
- use network-byte order.


> +static int prefix_list_proc_dump(char *buffer, char **start, off_t offset,
> +                        int length)
> +{
:

Please use seq_file.


Again, what I proposed was to store prefix information on fib with 
some flags to represent advertised by routers and give user-space 
the RA information using new rtattr (RTA_RA6INFO or something like that).

struct rta_ra6info {
     u32 rta_ra6flags;
};

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@xxxxxxxxxxxxxx>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA



<Prev in Thread] Current Thread [Next in Thread>