Re: [PATCH] bootp: add native DHCPv4 support

[Daniel Kiper]

On Mon, Jan 21, 2019 at 02:10:48PM +0000, Andre Przywara wrote:
> On Mon, 21 Jan 2019 13:02:08 +0100
> Daniel Kiper <address@hidden> 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 <address@hidden>
> > >
> > > 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 <address@hidden>
> > >
> > > ---
> > > 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