Re: [PATCH 1/3] Added net_bootp6 command

[Andrei Borzenkov <arvidjaar@gmail.com>]

В Tue, 19 May 2015 16:42:00 +0800
Michael Chang <mchang@suse.com> пишет:

> > 
> > > +#define GRUB_DHCPv6_OPTION_CLIENTID 1
> > > +#define GRUB_DHCPv6_OPTION_SERVERID 2
> > > +#define GRUB_DHCPv6_OPTION_IA_NA 3
> > > +#define GRUB_DHCPv6_OPTION_IAADDR 5
> > > +#define GRUB_DHCPv6_OPTION_ORO 6
> > > +#define GRUB_DHCPv6_OPTION_ELAPSED_TIME 8
> > > +#define GRUB_DHCPv6_OPTION_DNS_SERVERS 23
> > > +#define GRUB_DHCPv6_OPTION_BOOTFILE_URL 59
> > > +
> > 
> > enum please. These can also be anonymous, no need to invent fancy
> > names.
> 
> I don't understand. Do you want me to use enum type here rather than
> macro ? Where are the fancy names invented here ? 

Sorry I mean there is no need to name enume type. Just

enum
  {
     GRUB_DHCPv6_OPTION_CLIENTID = 1
    ...
  };

> > > +static void
> > > +get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet, grub_size_t size,
> > > +	grub_net_network_level_address_t **addr, grub_uint16_t *naddr)
> > > +{
> > > +  const struct grub_dhcpv6_option* popt;
> > > +  grub_uint16_t len, ln;
> > > +  const grub_uint8_t *pa;
> > > +  grub_net_network_level_address_t *la;
> > > +
> > > +  if (!addr || !naddr)
> > > +    {
> > > +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("bad argument for get_dhcpv6_dns_address"));
> > > +      return;
> > > +    }
> > > +
> > 
> > it is called exactly once and grub_errno is immediately reset to
> > GRUB_ERR_NONE. So what's the point of returning error at all?
> 
> Because  a function wants to report error for bad inputs, while it's
> caller wants to suppress the error message because dns information could
> be missing in packets and is allowed (not treated as error).
> 

Sure but it is called exactly once and result is not used. It is not
like this is library function, it is more of a convenience macro. For
the same reasons NULL check on input is probably redundant as well.

> > 
> > > +  if (len == 0 || len & 0xf)
> > > +    {
> > > +      grub_error (GRUB_ERR_IO, N_("invalid dns address length"));
> > > +      return;
> > > +    }
> > > +
> > 
> > same question about grub_error. May be grub_debug?
> 
> No grub_debug, maybe grub_dprintf ? Maybe we should keep it and use
> specific error number for option not found ? See below for details.
> 

Sure, I mean dprintf, sorry.

> > > +
> > > +  /* Follow elilo and edk2 that check for starting and ending delimiter '[..]'
> > > +     in which IPv6 server address is enclosed. */
> > > +  if (*ip_start != '[')
> > > +    ip_start = NULL;
> > > +  else
> > > +    ip_end = grub_strchr (++ip_start, ']');
> > > +
> > > +  if (!ip_start || !ip_end)
> > > +    {
> > > +      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square brackets"));
> > > +      goto cleanup;
> > > +    }
> > > +
> > 
> > RFC5970 says "Clients that have DNS implementations SHOULD support the
> > use of domain names in the URL" and in general string can also be valid
> > IPv4 address, nothing restricts it to IPv6. 
> 
> But the descriptions preceding it did explicitly say IPv6:
> 
> " If the host in the URL is expressed using an IPv6 address rather than
> a domain name, the address in the URL then MUST be enclosed in "[" and
> "]" characters, conforming to [RFC3986]. "
> 

it says "*if* it is using IPv6". Or at least so I understand it.

> > So I do not think you
> > should return error here, just return full string. I wish grub supported
> > IPv6 literals so we could just skip this check entirely.
> 
> OK. I'll return the string by assuming it's domain name.
> 
...
> > 
> > > +
> > > +  get_dhcpv6_dns_address (v6, size, &dns, &num_dns);
> > > +
> > > +  if (grub_errno)
> > > +    grub_errno = GRUB_ERR_NONE;
> > > +
> > 
> > This ignores legitimate errors like out of memory. As mentioned above,
> > does it need to set any grub_errno on its own? If not, errors should
> > not be ignored.
> 
> Because missing dns option is totally fine so that I don't want to treat
> it as error. We can use a specific error number for missing options and
> report other errors, but I can't find a suitable one for it (in
> include/grub/err.h).
> 

If missing DNS option is totally fine it should not be returned as
error. You already have indication (num_dns == 0) which is enough if
you want to inform user. Errors should be returned in case of e-h-h
errors :) 

Thanks! And sorry for delay.