Hi stsp,

Thanks for the patch! I think with a couple of changes we could make this a bit safer for BOOTP (or in general for pre-RFC 1531) clients, instead of appending options as if we had a DHCP client.

That is, a bit after that part, we have:

	if (opts[53].c[0] == DHCPDISCOVER) {
		info("DHCP: offer to discover");
		opts[53].s[0] = DHCPOFFER;
	} else if (opts[53].c[0] == DHCPREQUEST) {
		info("DHCP: ack to request");
		opts[53].s[0] = DHCPACK;
	} else {
		return -1;
	}

and, instead of making that 'return -1' essentially dead code, we could more accurately detect a BOOTP client by doing something like:

	} else if (!opts[53].clen)
		info("BOOTP: BOOTREQUEST to BOOTREPLY");
		bootp = true;
	} else {
		return -1;
	}

and then, if 'bootp' is true, just do the yiaddr part without appending options (also a goto would be okay if it keeps things simple). My fear is that we would otherwise answer a BOOTP client with options and cause it to malfunction. What do you think?

Yes, my patch was essentially a "slirp
does it this way and slirp works".
However I am not entirely sure if
something else is needed.

According to this:
http://www.tcpipguide.com/free/t_BOOTPMessageFormat.htm
bootp has not only yiaddr, but everything
else. More so, it has the vendor-specific
area, where I believe DHCP puts tags.
So I thought (which may be wrong) that
DHCP is fully derived from bootp, and
the difference is only in the tag fields
being unspecified for bootp.
You may argue that some "unknown" bootp
client could fail unless the vendor are
is zeroed out. But given that slirp does
not zero out the area, there is no such
fear.

(In reply to stsp from comment #2)
> According to this:
> http://www.tcpipguide.com/free/t_BOOTPMessageFormat.htm
> bootp has not only yiaddr, but everything
> else. More so, it has the vendor-specific
> area, where I believe DHCP puts tags.
Well, true, and RFC 1395 specifies almost all the options we add (except for option 121), *for BOOTP*, in a way that's compatible with DHCP.

> So I thought (which may be wrong) that
> DHCP is fully derived from bootp, and
> the difference is only in the tag fields
> being unspecified for bootp.
> You may argue that some "unknown" bootp
> client could fail unless the vendor are
> is zeroed out. But given that slirp does
> not zero out the area, there is no such
> fear.
Right -- let's keep the options then. But still, I think we should still merge that with the else if clauses as I mentioned in comment 1.

OK, please use this patch then:

diff --git a/dhcp.c b/dhcp.c
index d04648c..c1ac95e 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -323,8 +323,8 @@ int dhcp(const struct ctx *c, const struct pool *p)
        if (opts[53].c[0] == DHCPDISCOVER) {
                info("DHCP: offer to discover");
                opts[53].s[0] = DHCPOFFER;
-       } else if (opts[53].c[0] == DHCPREQUEST) {
-               info("DHCP: ack to request");
+       } else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) {
+               info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP");
                opts[53].s[0] = DHCPACK;
        } else {
                return -1;
diff --git a/tap.c b/tap.c

Thanks, it looks good to me... can you post it for review to the development mailing list (passt-dev@passt.top) using git send-email with proper tags (see examples from the archive, https://archives.passt.top/passt-dev/)?

If it's too much of a hassle I can also post it there with your nickname as author, let me know.

Done, patch sent.

Thanks!

> It would be interesting (maybe as part of bug 72)
> to have more details of your DHCP issues, yes.

OK, so to reproduce the DHCP problem, you
need to install this:
https://copr.fedorainfracloud.org/coprs/stsp/dosemu2/
(I am pretty sure you use fedora, so not
pointing to the ubuntu PPA).
You need to install dosemu2 and the
install-freedos package.
After doing so, you can run dosemu and
type `insfdusr` to call the installer.
If that succeeds, you can CD into
`c:\net\gopherus` and run gopherus.exe -
the gopher client. It should work fine
because slirp is used at that point.
After every start of gopherus.exe you
need to check and rempve `C:\tmp\w32dhcp.tmp`
or it will not initiate the DHCP session,
taking the cached values instead.

Now if that all works, you can enable passt.
You need to create `~/.dosemu/dosemurc`
with this content:
`$_netsock = "/tmp/passt_1.socket"`
And you need to start passt.
Now when you start gopherus.exe, you
should see that passt receives the
DHCP packets and sends replies, but
unfortunately gopherus ignores them.
It works perfectly with slirp, so I
suspect the bug is on a passt side.
But I can't be sure because I know
slirp has kludges for ancient DHCP
clients. It has a long history working
through all of them. :)

If you run dosemu with `-D9+Pn`
switch in cmdline, then in
~/.dosemu/boot.log you can see
the full dump of the network
packets. So you can even compare
the traffic with slirp and with
passt. One obvious thing is that
slirp replies to broadcast address,
whereas passt replies to unicast.
But I think I tried to change
that and it didn't help (unless
I changed it the wrong way somehow).

stsp, is this solved by the specific patch plus the solution for https://bugs.passt.top/show_bug.cgi?id=77 ?

Indeed.

I am pretty sure this was fixed.