Re: [PATCH] send router solicitation for ipv6 address autoconf

[Michael Chang]

On Sat, Nov 15, 2014 at 07:54:42PM +0300, Andrei Borzenkov wrote:
> В Thu, 13 Nov 2014 17:42:23 +0800
> Michael Chang <address@hidden> пишет:
> 
> > Many routers have long router advertisment interval configured by
> > default. The Neighbor Discovery protocol (RFC4861) has defined default
> > MaxRtrAdvInterval value as 600 seconds and
> > MinRtrAdvInterval as 0.33*MaxRtrAdvInterval. This makes
> > net_ipv6_autoconf fails more often than not as currently it passively
> > listens the RA message to perfom address autoconfiguration.
> > 
> > This patch tries to send router solicitation to overcome the problem of
> > long RA interval.
> > ---
> >  grub-core/net/icmp6.c |   81 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  grub-core/net/net.c   |    4 ++-
> >  include/grub/net/ip.h |    2 +
> >  3 files changed, 86 insertions(+), 1 deletions(-)
> > 
> > diff --git a/grub-core/net/icmp6.c b/grub-core/net/icmp6.c
> > index 796d549..87b264f 100644
> > --- a/grub-core/net/icmp6.c
> > +++ b/grub-core/net/icmp6.c
> > @@ -72,6 +72,11 @@ struct neighbour_advertise
> >    grub_uint64_t target[2];
> >  } GRUB_PACKED;
> >  
> > +struct router_solicit
> > +{
> > +  grub_uint32_t reserved;
> > +} GRUB_PACKED;
> > +
> >  enum
> >    {
> >      FLAG_SLAAC = 0x40
> > @@ -81,6 +86,7 @@ enum
> >    {
> >      ICMP6_ECHO = 128,
> >      ICMP6_ECHO_REPLY = 129,
> > +    ICMP6_ROUTER_SOLICIT = 133,
> >      ICMP6_ROUTER_ADVERTISE = 134,
> >      ICMP6_NEIGHBOUR_SOLICIT = 135,
> >      ICMP6_NEIGHBOUR_ADVERTISE = 136,
> > @@ -533,3 +539,78 @@ grub_net_icmp6_send_request (struct 
> > grub_net_network_level_interface *inf,
> >    grub_netbuff_free (nb);
> >    return err;
> >  }
> > +
> > +grub_err_t
> > +grub_net_icmp6_send_router_solicit (struct 
> > grub_net_network_level_interface *inf)
> > +{
> > +  struct grub_net_buff *nb;
> > +  grub_err_t err = GRUB_ERR_NONE;
> > +  grub_net_network_level_address_t multicast;
> > +  grub_net_link_level_address_t ll_multicast;
> > +  struct option_header *ohdr;
> > +  struct router_solicit *sol;
> > +  struct icmp_header *icmphr;
> > +
> > +  multicast.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> > +  multicast.ipv6[0] = grub_be_to_cpu64_compile_time (0xff02ULL << 48);
> > +  multicast.ipv6[1] = grub_be_to_cpu64_compile_time (0x02ULL);
> > +
> 
> Could you use cpu_to_be variant; it makes it less confusing (that is
> what we do here).

Agreed. But how about to define hton and ntoh family ?

> 
> > +  err = grub_net_link_layer_resolve (inf, &multicast, &ll_multicast);
> > +  if (err)
> > +    return err;
> > +
> > +  nb = grub_netbuff_alloc (sizeof (struct router_solicit)
> > +                      + sizeof (struct option_header)
> > +                      + 6
> > +                      + sizeof (struct icmp_header)
> > +                      + GRUB_NET_OUR_IPV6_HEADER_SIZE
> > +                      + GRUB_NET_MAX_LINK_HEADER_SIZE);
> > +  if (!nb)
> > +    return grub_errno;
> > +  err = grub_netbuff_reserve (nb,
> > +                         sizeof (struct router_solicit)
> > +                         + sizeof (struct option_header)
> > +                         + 6
> > +                         + sizeof (struct icmp_header)
> > +                         + GRUB_NET_OUR_IPV6_HEADER_SIZE
> > +                         + GRUB_NET_MAX_LINK_HEADER_SIZE);
> > +
> 
> Error check?

OK. I'll add it.

> 
> Otherwise personally I'm fine with it.

Thanks for review, I'll submit second version soon.

Regards,
Michael