On Mon, Jan 21, 2019 at 02:10:48PM +0000, Andre Przywara wrote: > On Mon, 21 Jan 2019 13:02:08 +0100 > Daniel Kiper <dki...@net-space.pl> wrote: > > Hi Daniel, > > thanks very much for your reply!
You are welcome. > > On Fri, Jan 11, 2019 at 03:59:34PM +0000, Andre Przywara wrote: > > > From: Andrei Borzenkov <arvidj...@gmail.com> > > > > > > This patch adds support for native DHCPv4 and removes requirement > > > for BOOTP compatibility support in DHCP server. > > > > > > There is no provision for selecting preferred server. We take the > > > first OFFER and try to REQUEST configuration from it. If NAK was > > > received, transaction is restarted, but if we hit timeout, > > > configuration fails. > > > > > > It also handles pure BOOTP reply (detected by lack of DHCP message > > > type option) and proceeds with configuration immedately. I could > > > not test it because I do not have pure BOOTP server. > > > > > > Because we need access to DHCP options in multiple places now, it > > > also factors out DHCP option processing, adding support for option > > > overload. > > > > > > Timeout handling is now per-interface, with independent timeouts for > > > both DISCOVER and REQUEST. It should make it more responsive if > > > answer was delayed initially. Total timeout remains the same as > > > originally, as well as backoff algorithm, but we poll in fixed > > > 200ms ticks so notice (successful) reply earlier. > > > > > > Failure to send packet to interface now does not terminate command > > > (and leaks memory) but rather simply ignores this interface and > > > continues with remaining ones if present. > > > > > > Finally it adds net_dhcp alias with intention to deprecate net_bootp > > > completely (it would be possible to make net_bootp to actually send > > > BOOTP packet if someone thinks it is required). > > > > > > Features not implements: > > > > > > - DHCP server selection. We take first DHCPOFFER or BOOTPREPLY. No > > > plans to implement. > > > > > > - DHCP option concatenation (RFC3396). I do not expect to hit it in > > > real life and it rather complicates implementation, so let's wait > > > for actual use case. > > > > > > - client identifier (RFC6842). So far we expect valid MAC address, > > > which should be enough to uniquely identify client. It is also not > > > clear how to generate unique client identifier if we ever need one. > > > > > > v2: change find_dhcp_option to use subscripts to avoid > > > signed/unsigned comparison warning. > > > > > > Should fix "may be used uninitialized" warning (although I > > > could not reproduce it). > > > > > > > I think that we should put Andrei's credits here. Probably we cannot > > add SOB without his approval. However, I am happy with anything else > > which does something in the similar fashion. > > Definitely it's his patch, and the From: line above should make this > clear and the patch would end up with his authorship in git. Ahhh... Right, this should suffice... > Happy to take any other tags, but as you mentioned, I can't just put > his SoB here. > > > > Signed-off-by: Andre Przywara <andre.przyw...@arm.com> > > > > > > --- > > > Hi, > > > > > > this patch is a rebased version of Andrei's work from 2016 > > > [1][2][3]. We have still this issue here where our company DHCP > > > server does not support the BOOTP protocol, so grub doesn't get a > > > valid IP address. > > > > > > I took v2 from Andrei of the list, and fixed the minor conflicts > > > during the rebase. I can confirm that this patch makes the > > > difference between DHCP working and not: > > > grub-master> net_bootp > > > error: couldn't autoconfigure efinet0. > > > ... > > > grub-patched> net_bootp > > > grub-patched> net_ls_addr > > > efinet0:dhcp 00:11:22:33:44:55 10.1.23.42 > > > > > > Also back in the days there were concerns about regressing > > > BOOTP-only servers. So I took the CMU bootpd[4], disabled the DHCP > > > extensions at compile time and still got IP addresses in both cases > > > (patched/unpatched). dhclient on Linux on the other hand was not > > > happy with that server and > > > > It seems to me that two sentences are glued here... > > > > > ignored the reply. I guess this is as close to a "BOOTP only > > > server" as we can get in 2019. > > > > I am OK with that. > > > > > I would be very happy if we can get this merged now. > > > > > > Please let me know if you need more information or any help on > > > this. > > > > Could you merge your comment with the commit message above? Please > > just refer to the current situation. If something historic should be > > referred please say clearly that it does not apply for today. > > Sure. > > > Is it possible to split this patch to smaller pieces? > > Possible: yes, though I am not sure I understand enough of it and grub > to do it properly. Will try my best, though, as soon as I find some > spare cycles to understand the patch a bit better. Is it matter of days, weeks or months? > > Some nitpicks below... > > > > > Thanks! > > > Andre. > > > > > > P.S. The original patch didn't have a Signed-off-by:, so I kept it > > > that way. Added mine for legal reason, feel free to drop it. > > > > > > [1] > > > http://lists.gnu.org/archive/html/grub-devel/2016-01/msg00035.html > > > [2] > > > http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00276.html > > > [3] > > > http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00066.html > > > [4] https://packages.debian.org/jessie/bootp > > > > > > grub-core/net/bootp.c | 759 > > > ++++++++++++++++++++++++++++-------------- grub-core/net/ip.c > > > | 2 +- include/grub/net.h | 14 +- > > > 3 files changed, 528 insertions(+), 247 deletions(-) > > > > > > diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c > > > index 9e2fdb795..890df715b 100644 > > > --- a/grub-core/net/bootp.c > > > +++ b/grub-core/net/bootp.c > > > > If we add DHCP support here then I think we should change file name in > > the following patch. > > Yeah, makes some sense. > > > > @@ -25,25 +25,107 @@ > > > #include <grub/net/udp.h> > > > #include <grub/datetime.h> > > > > > > -static void > > > -parse_dhcp_vendor (const char *name, const void *vend, int limit, > > > int *mask) +struct grub_dhcp_discover_options > > > + { > > > > Drop spaces before "{". > > Yes, I saw this, fixed it, then reverted it because I couldn't find an > explicit coding style and didn't want to mess with Andrei's original > work needlessly. > Consider it fixed. > > The rest of the comments make sense, I will address them eventually, > unless someone (Andrei?) screams... Thanks a lot! I am looking forward for next version of this patch. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
