Re: [systemd-devel] [PATCH] sd-dhcp-client: fix REBOOT state handling

2014-11-19 Thread Patrik Flykt 
On Tue, 2014-11-18 at 18:41 +0100, Tom Gundersen wrote:

 (but cc'ing Patrik just to make sure)

  RFC 2131:
 
   secs   2  Filled in by client, seconds elapsed since client
 began address acquisition or renewal process.

  @@ -422,7 +423,15 @@ static int client_message_init(sd_dhcp_client *client, 
  DHCPPacket **ret,
 
   /* Although 'secs' field is a SHOULD in RFC 2131, certain DHCP 
  servers
  refuse to issue an DHCP lease if 'secs' is set to zero */
  -packet-dhcp.secs = htobe16(client-secs);
  +r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
  time_now);
  +if (r  0)
  +return r;
  +assert(time_now = client-start_time);
  +
  +/* seconds between sending first and last DISCOVER
  + * must always be strictly positive to deal with broken servers */
  +secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 1;
  +packet-dhcp.secs = htobe16(secs);

The previous code assumed that resending a message mean really sending
an identical one in place of the lost one. This change actually nags on
the other end that time has really passed, be it a clue to a DHCP relay
that it should do something else than before. Looking at the RFC snippet
and the patch, I think this is what the RFC actually tried to say.

Cheers,

Patrik

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] sd-dhcp-client: fix REBOOT state handling

2014-11-18 Thread Dan Williams 
On Tue, 2014-11-04 at 11:20 -0600, Dan Williams wrote:
 client-secs wasn't getting set in the REBOOT state, causing
 an assertion.  REBOOT should work the same way as INIT, per
 RFC 2131:
 
  secs   2  Filled in by client, seconds elapsed since client
began address acquisition or renewal process.
 
 REBOOT is necessary because some DHCP servers (eg on
 home routers) do not hand back the same IP address unless the
 'ciaddr' field is filled with that address, which DISCOVER
 cannot do per the RFCs.  This leads to multiple leases
 on machine reboot or DHCP client restart.

Anyone had a chance to review this patch?

 ---
  src/libsystemd-network/sd-dhcp-client.c | 31 +--
  1 file changed, 13 insertions(+), 18 deletions(-)
 
 diff --git a/src/libsystemd-network/sd-dhcp-client.c 
 b/src/libsystemd-network/sd-dhcp-client.c
 index a8ec654..300fc38 100644
 --- a/src/libsystemd-network/sd-dhcp-client.c
 +++ b/src/libsystemd-network/sd-dhcp-client.c
 @@ -89,7 +89,6 @@ struct sd_dhcp_client {
  uint32_t mtu;
  uint32_t xid;
  usec_t start_time;
 -uint16_t secs;
  unsigned int attempt;
  usec_t request_sent;
  sd_event_source *timeout_t1;
 @@ -399,10 +398,12 @@ static int client_message_init(sd_dhcp_client *client, 
 DHCPPacket **ret,
  _cleanup_free_ DHCPPacket *packet;
  size_t optlen, optoffset, size;
  be16_t max_size;
 +usec_t time_now;
 +uint16_t secs;
  int r;
  
  assert(client);
 -assert(client-secs);
 +assert(client-start_time);
  assert(ret);
  assert(_optlen);
  assert(_optoffset);
 @@ -422,7 +423,15 @@ static int client_message_init(sd_dhcp_client *client, 
 DHCPPacket **ret,
  
  /* Although 'secs' field is a SHOULD in RFC 2131, certain DHCP 
 servers
 refuse to issue an DHCP lease if 'secs' is set to zero */
 -packet-dhcp.secs = htobe16(client-secs);
 +r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
 time_now);
 +if (r  0)
 +return r;
 +assert(time_now = client-start_time);
 +
 +/* seconds between sending first and last DISCOVER
 + * must always be strictly positive to deal with broken servers */
 +secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 1;
 +packet-dhcp.secs = htobe16(secs);
  
  /* RFC2132 section 4.1
 A client that cannot receive unicast IP datagrams until its 
 protocol
 @@ -529,24 +538,12 @@ static int dhcp_client_send_raw(sd_dhcp_client *client, 
 DHCPPacket *packet,
  static int client_send_discover(sd_dhcp_client *client) {
  _cleanup_free_ DHCPPacket *discover = NULL;
  size_t optoffset, optlen;
 -usec_t time_now;
  int r;
  
  assert(client);
  assert(client-state == DHCP_STATE_INIT ||
 client-state == DHCP_STATE_SELECTING);
  
 -/* See RFC2131 section 4.4.1 */
 -
 -r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
 time_now);
 -if (r  0)
 -return r;
 -assert(time_now = client-start_time);
 -
 -/* seconds between sending first and last DISCOVER
 - * must always be strictly positive to deal with broken servers */
 -client-secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 
 1;
 -
  r = client_message_init(client, discover, DHCP_DISCOVER,
  optlen, optoffset);
  if (r  0)
 @@ -963,10 +960,8 @@ static int client_start(sd_dhcp_client *client) {
  }
  client-fd = r;
  
 -if (client-state == DHCP_STATE_INIT) {
 +if (client-state == DHCP_STATE_INIT || client-state == 
 DHCP_STATE_INIT_REBOOT)
  client-start_time = now(clock_boottime_or_monotonic());
 -client-secs = 0;
 -}
  
  return client_initialize_events(client, client_receive_message_raw);
  }


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] sd-dhcp-client: fix REBOOT state handling

2014-11-18 Thread Tom Gundersen 
Applied. Thanks!

(but cc'ing Patrik just to make sure)

On Tue, Nov 4, 2014 at 6:20 PM, Dan Williams d...@redhat.com wrote:
 client-secs wasn't getting set in the REBOOT state, causing
 an assertion.  REBOOT should work the same way as INIT, per
 RFC 2131:

  secs   2  Filled in by client, seconds elapsed since client
began address acquisition or renewal process.

 REBOOT is necessary because some DHCP servers (eg on
 home routers) do not hand back the same IP address unless the
 'ciaddr' field is filled with that address, which DISCOVER
 cannot do per the RFCs.  This leads to multiple leases
 on machine reboot or DHCP client restart.
 ---
  src/libsystemd-network/sd-dhcp-client.c | 31 +--
  1 file changed, 13 insertions(+), 18 deletions(-)

 diff --git a/src/libsystemd-network/sd-dhcp-client.c 
 b/src/libsystemd-network/sd-dhcp-client.c
 index a8ec654..300fc38 100644
 --- a/src/libsystemd-network/sd-dhcp-client.c
 +++ b/src/libsystemd-network/sd-dhcp-client.c
 @@ -89,7 +89,6 @@ struct sd_dhcp_client {
  uint32_t mtu;
  uint32_t xid;
  usec_t start_time;
 -uint16_t secs;
  unsigned int attempt;
  usec_t request_sent;
  sd_event_source *timeout_t1;
 @@ -399,10 +398,12 @@ static int client_message_init(sd_dhcp_client *client, 
 DHCPPacket **ret,
  _cleanup_free_ DHCPPacket *packet;
  size_t optlen, optoffset, size;
  be16_t max_size;
 +usec_t time_now;
 +uint16_t secs;
  int r;

  assert(client);
 -assert(client-secs);
 +assert(client-start_time);
  assert(ret);
  assert(_optlen);
  assert(_optoffset);
 @@ -422,7 +423,15 @@ static int client_message_init(sd_dhcp_client *client, 
 DHCPPacket **ret,

  /* Although 'secs' field is a SHOULD in RFC 2131, certain DHCP 
 servers
 refuse to issue an DHCP lease if 'secs' is set to zero */
 -packet-dhcp.secs = htobe16(client-secs);
 +r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
 time_now);
 +if (r  0)
 +return r;
 +assert(time_now = client-start_time);
 +
 +/* seconds between sending first and last DISCOVER
 + * must always be strictly positive to deal with broken servers */
 +secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 1;
 +packet-dhcp.secs = htobe16(secs);

  /* RFC2132 section 4.1
 A client that cannot receive unicast IP datagrams until its 
 protocol
 @@ -529,24 +538,12 @@ static int dhcp_client_send_raw(sd_dhcp_client *client, 
 DHCPPacket *packet,
  static int client_send_discover(sd_dhcp_client *client) {
  _cleanup_free_ DHCPPacket *discover = NULL;
  size_t optoffset, optlen;
 -usec_t time_now;
  int r;

  assert(client);
  assert(client-state == DHCP_STATE_INIT ||
 client-state == DHCP_STATE_SELECTING);

 -/* See RFC2131 section 4.4.1 */
 -
 -r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
 time_now);
 -if (r  0)
 -return r;
 -assert(time_now = client-start_time);
 -
 -/* seconds between sending first and last DISCOVER
 - * must always be strictly positive to deal with broken servers */
 -client-secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 
 1;
 -
  r = client_message_init(client, discover, DHCP_DISCOVER,
  optlen, optoffset);
  if (r  0)
 @@ -963,10 +960,8 @@ static int client_start(sd_dhcp_client *client) {
  }
  client-fd = r;

 -if (client-state == DHCP_STATE_INIT) {
 +if (client-state == DHCP_STATE_INIT || client-state == 
 DHCP_STATE_INIT_REBOOT)
  client-start_time = now(clock_boottime_or_monotonic());
 -client-secs = 0;
 -}

  return client_initialize_events(client, client_receive_message_raw);
  }
 --
 1.9.3


 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] sd-dhcp-client: fix REBOOT state handling

2014-11-04 Thread Dan Williams 
client-secs wasn't getting set in the REBOOT state, causing
an assertion.  REBOOT should work the same way as INIT, per
RFC 2131:

 secs   2  Filled in by client, seconds elapsed since client
   began address acquisition or renewal process.

REBOOT is necessary because some DHCP servers (eg on
home routers) do not hand back the same IP address unless the
'ciaddr' field is filled with that address, which DISCOVER
cannot do per the RFCs.  This leads to multiple leases
on machine reboot or DHCP client restart.
---
 src/libsystemd-network/sd-dhcp-client.c | 31 +--
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/libsystemd-network/sd-dhcp-client.c 
b/src/libsystemd-network/sd-dhcp-client.c
index a8ec654..300fc38 100644
--- a/src/libsystemd-network/sd-dhcp-client.c
+++ b/src/libsystemd-network/sd-dhcp-client.c
@@ -89,7 +89,6 @@ struct sd_dhcp_client {
 uint32_t mtu;
 uint32_t xid;
 usec_t start_time;
-uint16_t secs;
 unsigned int attempt;
 usec_t request_sent;
 sd_event_source *timeout_t1;
@@ -399,10 +398,12 @@ static int client_message_init(sd_dhcp_client *client, 
DHCPPacket **ret,
 _cleanup_free_ DHCPPacket *packet;
 size_t optlen, optoffset, size;
 be16_t max_size;
+usec_t time_now;
+uint16_t secs;
 int r;
 
 assert(client);
-assert(client-secs);
+assert(client-start_time);
 assert(ret);
 assert(_optlen);
 assert(_optoffset);
@@ -422,7 +423,15 @@ static int client_message_init(sd_dhcp_client *client, 
DHCPPacket **ret,
 
 /* Although 'secs' field is a SHOULD in RFC 2131, certain DHCP servers
refuse to issue an DHCP lease if 'secs' is set to zero */
-packet-dhcp.secs = htobe16(client-secs);
+r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
time_now);
+if (r  0)
+return r;
+assert(time_now = client-start_time);
+
+/* seconds between sending first and last DISCOVER
+ * must always be strictly positive to deal with broken servers */
+secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 1;
+packet-dhcp.secs = htobe16(secs);
 
 /* RFC2132 section 4.1
A client that cannot receive unicast IP datagrams until its protocol
@@ -529,24 +538,12 @@ static int dhcp_client_send_raw(sd_dhcp_client *client, 
DHCPPacket *packet,
 static int client_send_discover(sd_dhcp_client *client) {
 _cleanup_free_ DHCPPacket *discover = NULL;
 size_t optoffset, optlen;
-usec_t time_now;
 int r;
 
 assert(client);
 assert(client-state == DHCP_STATE_INIT ||
client-state == DHCP_STATE_SELECTING);
 
-/* See RFC2131 section 4.4.1 */
-
-r = sd_event_now(client-event, clock_boottime_or_monotonic(), 
time_now);
-if (r  0)
-return r;
-assert(time_now = client-start_time);
-
-/* seconds between sending first and last DISCOVER
- * must always be strictly positive to deal with broken servers */
-client-secs = ((time_now - client-start_time) / USEC_PER_SEC) ? : 1;
-
 r = client_message_init(client, discover, DHCP_DISCOVER,
 optlen, optoffset);
 if (r  0)
@@ -963,10 +960,8 @@ static int client_start(sd_dhcp_client *client) {
 }
 client-fd = r;
 
-if (client-state == DHCP_STATE_INIT) {
+if (client-state == DHCP_STATE_INIT || client-state == 
DHCP_STATE_INIT_REBOOT)
 client-start_time = now(clock_boottime_or_monotonic());
-client-secs = 0;
-}
 
 return client_initialize_events(client, client_receive_message_raw);
 }
-- 
1.9.3


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel