klibc - Jun 2008 - PATCH: ipconfig may accept DHCPOFFER as DHCPACK

Hello,

I found a bug in ipconfig and I'm sending a proposed patch for it.
I've only seen it happen in 2 dhcp-server environments. Scenario:
ipconfig sends a DHCP_DISCOVER,
server A answers with a DHCP_OFFER,
server B answers with a DHCP_OFFER,
ipconfig sends a DHCP_REQUEST for server A,
ipconfig accepts the DHCP_OFFER from server B instead of DHCP_ACK from
server A. <== BUG

The reason for this was that in dhcp_parse(), DHCPOFFER and DHCPACK
returned the same value (=1). So I had to modify the return values,
and I thought it would be better to return the constants DHCPOFFER,
DHCPACK etc instead of 1,2,3... I tried to keep the changes as minimal
as possible.

I'm not experienced with the diff program, so please excuse the way
I'm sending the patch: it's the result from the "diff main.c
../ipconfig.old/main.c" and "diff dhcp_proto.c
../ipconfig.old/dhcp_proto.c" commands. The version I used was klibc
1.5.10, Release: 1.

Kind regards,
Alkis

diff main.c ../ipconfig.old/main.c:
195c195
< 		case DHCPOFFER:	/* Offer received */
---
> 		case 1:	/* Offer received */
208c208
< 		case DHCPACK:	/* ACK received */
---
> 		case 1:	/* ACK received */
211c211
< 		case DHCPNAK:	/* NAK received */
---
> 		case 2:	/* NAK received */

diff dhcp_proto.c ../ipconfig.old/dhcp_proto.c:
74,78d73
<  * Returns:
<  * 0 = Not handled
<  * 2 = DHCPOFFER (from dhcp_proto.h)
<  * 5 = DHCPACK
<  * 6 = DHCPNACK
109,110c104,105
< 		ret = bootp_parse(dev, hdr, exts, extlen) ? DHCPOFFER : 0;
< 		if (ret == DHCPOFFER && serverid != INADDR_NONE)
---
> 		ret = bootp_parse(dev, hdr, exts, extlen);
> 		if (ret == 1 && serverid != INADDR_NONE)
116c111
< 		ret = bootp_parse(dev, hdr, exts, extlen) ? DHCPACK : 0;
---
> 		ret = bootp_parse(dev, hdr, exts, extlen);
121c116
< 		ret = DHCPNAK;
---
> 		ret = 2;
130,135d124
<  * Returns:
<  *-1 = Error in packet_recv
<  * 0 = Not handled
<  * 2 = DHCPOFFER (from dhcp_proto.h)
<  * 5 = DHCPACK
<  * 6 = DHCPNACK

????? ???????????? wrote:
> 
> I'm not experienced with the diff program, so please excuse the way
> I'm sending the patch: it's the result from the "diff main.c
> ../ipconfig.old/main.c" and "diff dhcp_proto.c
> ../ipconfig.old/dhcp_proto.c" commands. The version I used was klibc
> 1.5.10, Release: 1.
> 
Please do:

diff -ur ../ipconfig.old . > output.diff

... instead.

	-hpa

????? ???????????? wrote:
> 
> I'm not experienced with the diff program, so please excuse the way
> I'm sending the patch: it's the result from the "diff main.c
> ../ipconfig.old/main.c" and "diff dhcp_proto.c
> ../ipconfig.old/dhcp_proto.c" commands. The version I used was klibc
> 1.5.10, Release: 1.
> 
Oh, yes, I *cannot* accept a patch from you without a Signed-off-by: 
line.  Please see the following document:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;hb=HEAD

	-hpa

Thank you Peter,

diff -ur ../ipconfig.old/dhcp_proto.c ./dhcp_proto.c
--- ../ipconfig.old/dhcp_proto.c	2008-06-14 01:52:11.000000000 +0300
+++ ./dhcp_proto.c	2008-06-14 02:28:32.000000000 +0300
@@ -71,6 +71,11 @@

 /*
  * Parse a DHCP response packet
+ * Returns:
+ * 0 = Not handled
+ * 2 = DHCPOFFER (from dhcp_proto.h)
+ * 5 = DHCPACK
+ * 6 = DHCPNACK
  */
 static int
 dhcp_parse(struct netdev *dev, struct bootp_hdr *hdr, uint8_t * exts,
int extlen)
@@ -101,19 +106,19 @@

 	switch (type) {
 	case DHCPOFFER:
-		ret = bootp_parse(dev, hdr, exts, extlen);
-		if (ret == 1 && serverid != INADDR_NONE)
+		ret = bootp_parse(dev, hdr, exts, extlen) ? DHCPOFFER : 0;
+		if (ret == DHCPOFFER && serverid != INADDR_NONE)
 			dev->serverid = serverid;
 		DEBUG(("\n   dhcp offer\n"));
 		break;

 	case DHCPACK:
-		ret = bootp_parse(dev, hdr, exts, extlen);
+		ret = bootp_parse(dev, hdr, exts, extlen) ? DHCPACK : 0;
 		DEBUG(("\n   dhcp ack\n"));
 		break;

 	case DHCPNAK:
-		ret = 2;
+		ret = DHCPNAK;
 		DEBUG(("\n   dhcp nak\n"));
 		break;
 	}
@@ -122,6 +127,12 @@

 /*
  * Receive and parse a DHCP packet
+ * Returns:
+ *-1 = Error in packet_recv
+ * 0 = Not handled
+ * 2 = DHCPOFFER (from dhcp_proto.h)
+ * 5 = DHCPACK
+ * 6 = DHCPNACK
  */
 static int dhcp_recv(struct netdev *dev)
 {
diff -ur ../ipconfig.old/main.c ./main.c
--- ../ipconfig.old/main.c	2008-06-14 01:52:11.000000000 +0300
+++ ./main.c	2008-06-14 02:30:45.000000000 +0300
@@ -192,7 +192,7 @@
 		case -1:
 			s->state = DEVST_ERROR;
 			break;
-		case 1:	/* Offer received */
+		case DHCPOFFER:	/* Offer received */
 			s->state = DEVST_DHCPREQ;
 			dhcp_send_request(s->dev);
 			break;
@@ -205,10 +205,10 @@
 		case -1:	/* error */
 			s->state = DEVST_ERROR;
 			break;
-		case 1:	/* ACK received */
+		case DHCPACK:	/* ACK received */
 			s->state = DEVST_COMPLETE;
 			break;
-		case 2:	/* NAK received */
+		case DHCPNAK:	/* NAK received */
 			s->state = DEVST_DHCPDISC;
 			break;
 		}

????? ???????????? wrote:
> Thank you Peter,
Much better, but I still need a Signed-off-by: Line.

All you need to do is to send the patch again in a separate email, with 
a description at the top and then the line:

Signed-off-by: ????? ???????????? <alkisg at gmail.com>

... followed by the patch itself.

This is important for legal reasons.

Sorry,

	-hpa

On Sat, 14 Jun 2008, ????? ???????????? wrote:
> Thank you Peter,
> 
> diff -ur ../ipconfig.old/dhcp_proto.c ./dhcp_proto.c
> --- ../ipconfig.old/dhcp_proto.c	2008-06-14 01:52:11.000000000 +0300
> +++ ./dhcp_proto.c	2008-06-14 02:28:32.000000000 +0300
cool thanks.

for next time do your a favour and use git directly:
# fetch latest git repo
git clone git://git.kernel.org/pub/scm/libs/klibc/klibc.git

# create a branch for your work
git checkout -b ipconfig
.. (aka hack compile test)
# commit working stuff with signed off
git commit -a -s
# let recheck latest patch
git show
# eventually change something on the message
git commit ammend
# export patch for emails
git format-patch master..ipconfig
# send them off
git send-email --to bla --cc bli 0001-...patch

happy hacking

-- 
maks