В Tue, 19 May 2015 16:42:00 +0800
Michael Chang <mch...@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.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
