From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Yyb93-00031R-P2 for mharc-grub-devel@gnu.org; Sat, 30 May 2015 03:25:25 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48254) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yyb91-000313-93 for grub-devel@gnu.org; Sat, 30 May 2015 03:25:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yyb8w-0006Db-8F for grub-devel@gnu.org; Sat, 30 May 2015 03:25:23 -0400 Received: from mail-la0-x233.google.com ([2a00:1450:4010:c03::233]:35876) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yyb8v-0006DX-Rv for grub-devel@gnu.org; Sat, 30 May 2015 03:25:18 -0400 Received: by lagv1 with SMTP id v1so70207390lag.3 for ; Sat, 30 May 2015 00:25:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=HLIyvtwVRQl9d5Jib2ihRys5YAZzAfFcQigaSE3U4xo=; b=ChGFBfaLyaaESgH6IKXT4owuG8zGOcfrSw3Xsmz9oUbeI6ENhD1f+Qa6e6A0XrQA6L xj+Z3MJUIzhdorTci4HSXKF8py243awFN8AxWLL8WDPiXeqsUERBwB4FaJS1htuv2d6F Fb2qfhdBmshPwnpbezIHhhPcChNe0YTV6PthEYEhXOsDxVT3DW9gw49+e2L1QaXDo69+ WyljrUQ4x9/5yGp7tdARcUHR004SYxrlcJCaZvgVLlsYevxr5aKEDOLNsR8hXu9d8Y6e VXy0rltN5KaCimDmK0/zAIclW38GBNGs8jC4Igq4LrtE3vD9QYT3rPamGLRqCK9rASr8 wj7A== X-Received: by 10.112.144.69 with SMTP id sk5mr11624734lbb.6.1432970717227; Sat, 30 May 2015 00:25:17 -0700 (PDT) Received: from opensuse.site (ppp91-76-14-38.pppoe.mtu-net.ru. [91.76.14.38]) by mx.google.com with ESMTPSA id 4sm2104699lai.36.2015.05.30.00.25.16 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 30 May 2015 00:25:16 -0700 (PDT) Date: Sat, 30 May 2015 10:25:15 +0300 From: Andrei Borzenkov To: Michael Chang Subject: Re: [PATCH 1/3] Added net_bootp6 command Message-ID: <20150530102515.5cc45d91@opensuse.site> In-Reply-To: <20150519084200.GB2363@linux-dsax.tai.apac.novell.com> References: <1431420590-7245-1-git-send-email-mchang@suse.com> <1431420590-7245-2-git-send-email-mchang@suse.com> <20150515092606.3efdf89e@opensuse.site> <20150519084200.GB2363@linux-dsax.tai.apac.novell.com> X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.27; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::233 Cc: grub-devel@gnu.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 May 2015 07:25:24 -0000 =D0=92 Tue, 19 May 2015 16:42:00 +0800 Michael Chang =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > >=20 > > > +#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 > > > + > >=20 > > enum please. These can also be anonymous, no need to invent fancy > > names. >=20 > I don't understand. Do you want me to use enum type here rather than > macro ? Where are the fancy names invented here ?=20 Sorry I mean there is no need to name enume type. Just enum { GRUB_DHCPv6_OPTION_CLIENTID =3D 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_dh= cpv6_dns_address")); > > > + return; > > > + } > > > + > >=20 > > 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? >=20 > 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). >=20 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. > >=20 > > > + if (len =3D=3D 0 || len & 0xf) > > > + { > > > + grub_error (GRUB_ERR_IO, N_("invalid dns address length")); > > > + return; > > > + } > > > + > >=20 > > same question about grub_error. May be grub_debug? >=20 > No grub_debug, maybe grub_dprintf ? Maybe we should keep it and use > specific error number for option not found ? See below for details. >=20 Sure, I mean dprintf, sorry. > > > + > > > + /* Follow elilo and edk2 that check for starting and ending delimi= ter '[..]' > > > + in which IPv6 server address is enclosed. */ > > > + if (*ip_start !=3D '[') > > > + ip_start =3D NULL; > > > + else > > > + ip_end =3D grub_strchr (++ip_start, ']'); > > > + > > > + if (!ip_start || !ip_end) > > > + { > > > + grub_error (GRUB_ERR_IO, N_("IPv6-address not in square bracke= ts")); > > > + goto cleanup; > > > + } > > > + > >=20 > > 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.=20 >=20 > But the descriptions preceding it did explicitly say IPv6: >=20 > " 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]. " >=20 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. >=20 > OK. I'll return the string by assuming it's domain name. >=20 ... > >=20 > > > + > > > + get_dhcpv6_dns_address (v6, size, &dns, &num_dns); > > > + > > > + if (grub_errno) > > > + grub_errno =3D GRUB_ERR_NONE; > > > + > >=20 > > 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. >=20 > 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). >=20 If missing DNS option is totally fine it should not be returned as error. You already have indication (num_dns =3D=3D 0) which is enough if you want to inform user. Errors should be returned in case of e-h-h errors :)=20 Thanks! And sorry for delay.