Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-20 Thread Lennart Poettering 
On Thu, 19.06.14 14:52, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:

 On Wed, 2014-06-18 at 16:25 +0200, Zbigniew Jędrzejewski-Szmek wrote:
  On Wed, Jun 18, 2014 at 07:05:35AM -0700, Filipe Brandenburger wrote:
   On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
   zbys...@in.waw.pl wrote:
+if (client-fd  0)
+safe_close(client-fd);
+client-fd = -1;
client-fd = safe_close(client-fd);
   
That's what safe_close is for :)
   
   And shouldn't the check be for client-fd = 0? Zero is a valid file 
   descriptor.
  Yeah... but note that safe_close already has the fd = 0 check, so the
  replacement line replaces the if too.
 
 If I omit the client-fd  0 check I get the following in the test case:
 
 Assertion 'close_nointr(fd) != -EBADF' failed at src/shared/util.c:205,
 function safe_close(). Aborting.

safe_close() is a call that will ignore all kinds of errors except one:
EBADF. THe rationale for that is that the kernel has these weird
semantics that close() might fail with errors like EIO or whatever else
if something could't be written to disk or so, but the fd will still be
closed. Hence, you can invoke safe_close() in all those cases where you
just want to get rid of an fd, and don't care about anything else. Now,
it will only trip up on one specific problem which always indicates a
programming error: when close() returns EBADF, since that is the error
that is returned when you invoke close() on an fd that doesn't exist.

Putting this all together: safe_close() is basically your one stop
solution to getting rid of fds, and even updating your variable you
store it in:

fd = safe_close(fd);

safe_close() always returns -1, always gets rid of the fd, will be a NOP
if the fd is  0. Will never fail. However, it if you invoke it the only
way that is a real programming error which is with an already-closed fd
or a never-opened fd, then it will assert() and die.

Hope this makes sense.

Or long words short: if the code tripped up like yours above, this is no
indication that safe_close() wasn't right. Instead it's an indication
that you are passing it a rubbish fd.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-20 Thread Zbigniew Jędrzejewski-Szmek 
On Fri, Jun 20, 2014 at 07:45:39PM +0200, Lennart Poettering wrote:
 On Thu, 19.06.14 14:52, Patrik Flykt (patrik.fl...@linux.intel.com) wrote:
 
  On Wed, 2014-06-18 at 16:25 +0200, Zbigniew Jędrzejewski-Szmek wrote:
   On Wed, Jun 18, 2014 at 07:05:35AM -0700, Filipe Brandenburger wrote:
On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
zbys...@in.waw.pl wrote:
 +if (client-fd  0)
 +safe_close(client-fd);
 +client-fd = -1;
 client-fd = safe_close(client-fd);

 That's what safe_close is for :)

And shouldn't the check be for client-fd = 0? Zero is a valid file 
descriptor.
   Yeah... but note that safe_close already has the fd = 0 check, so the
   replacement line replaces the if too.
  
  If I omit the client-fd  0 check I get the following in the test case:
  
  Assertion 'close_nointr(fd) != -EBADF' failed at src/shared/util.c:205,
  function safe_close(). Aborting.
 
 safe_close() is a call that will ignore all kinds of errors except one:
 EBADF. THe rationale for that is that the kernel has these weird
 semantics that close() might fail with errors like EIO or whatever else
 if something could't be written to disk or so, but the fd will still be
 closed. Hence, you can invoke safe_close() in all those cases where you
 just want to get rid of an fd, and don't care about anything else. Now,
 it will only trip up on one specific problem which always indicates a
 programming error: when close() returns EBADF, since that is the error
 that is returned when you invoke close() on an fd that doesn't exist.
 
 Putting this all together: safe_close() is basically your one stop
 solution to getting rid of fds, and even updating your variable you
 store it in:
 
 fd = safe_close(fd);
 
 safe_close() always returns -1, always gets rid of the fd, will be a NOP
 if the fd is  0. Will never fail. However, it if you invoke it the only
 way that is a real programming error which is with an already-closed fd
 or a never-opened fd, then it will assert() and die.
 
 Hope this makes sense.
 
 Or long words short: if the code tripped up like yours above, this is no
 indication that safe_close() wasn't right. Instead it's an indication
 that you are passing it a rubbish fd.
Yeah, fixed in c806ffb.

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

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-19 Thread Patrik Flykt 
On Wed, 2014-06-18 at 16:25 +0200, Zbigniew Jędrzejewski-Szmek wrote:
 On Wed, Jun 18, 2014 at 07:05:35AM -0700, Filipe Brandenburger wrote:
  On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
  zbys...@in.waw.pl wrote:
   +if (client-fd  0)
   +safe_close(client-fd);
   +client-fd = -1;
   client-fd = safe_close(client-fd);
  
   That's what safe_close is for :)
  
  And shouldn't the check be for client-fd = 0? Zero is a valid file 
  descriptor.
 Yeah... but note that safe_close already has the fd = 0 check, so the
 replacement line replaces the if too.

If I omit the client-fd  0 check I get the following in the test case:

Assertion 'close_nointr(fd) != -EBADF' failed at src/shared/util.c:205,
function safe_close(). Aborting.


Cheers,

Patrik

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

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-19 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, Jun 19, 2014 at 02:52:06PM +0300, Patrik Flykt wrote:
 On Wed, 2014-06-18 at 16:25 +0200, Zbigniew Jędrzejewski-Szmek wrote:
  On Wed, Jun 18, 2014 at 07:05:35AM -0700, Filipe Brandenburger wrote:
   On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
   zbys...@in.waw.pl wrote:
+if (client-fd  0)
+safe_close(client-fd);
+client-fd = -1;
client-fd = safe_close(client-fd);
   
That's what safe_close is for :)
   
   And shouldn't the check be for client-fd = 0? Zero is a valid file 
   descriptor.
  Yeah... but note that safe_close already has the fd = 0 check, so the
  replacement line replaces the if too.
 
 If I omit the client-fd  0 check I get the following in the test case:
 
 Assertion 'close_nointr(fd) != -EBADF' failed at src/shared/util.c:205,
 function safe_close(). Aborting.
Hm, is client-fd == 0? Most likely this means that it needs to be initialized
to -1.

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

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-18 Thread Zbigniew Jędrzejewski-Szmek 
On Fri, Jun 13, 2014 at 04:45:02PM +0300, Patrik Flykt wrote:
 Implement the initial functionality used for creating a DHCPv6 Solicit
 message containing the needed options and send it to the DHCPv6
 broadcast address. Increase the sent message count and ensure that
 the Solicit Initial Retransmission Time is strictly greater than
 the Solicitation IRT as described in RFC 3315, section 17.1.2.
 ---
  src/libsystemd-network/dhcp6-internal.h  |   4 ++
  src/libsystemd-network/sd-dhcp6-client.c | 111 
 +++
  2 files changed, 115 insertions(+)
 
 diff --git a/src/libsystemd-network/dhcp6-internal.h 
 b/src/libsystemd-network/dhcp6-internal.h
 index 7a491fb..31f5bd2 100644
 --- a/src/libsystemd-network/dhcp6-internal.h
 +++ b/src/libsystemd-network/dhcp6-internal.h
 @@ -27,6 +27,7 @@
  #include sparse-endian.h
  #include sd-event.h
  #include list.h
 +#include macro.h
  
  typedef struct DHCP6Address DHCP6Address;
  
 @@ -69,3 +70,6 @@ int dhcp6_option_parse(uint8_t **buf, size_t *buflen, 
 uint16_t *optcode,
  int dhcp6_network_bind_udp_socket(int index, struct in6_addr *address);
  int dhcp6_network_send_udp_socket(int s, struct in6_addr *address,
const void *packet, size_t len);
 +
 +const char *dhcp6_message_type_to_string(int s) _const_;
 +int dhcp6_message_type_from_string(const char *s) _pure_;
 diff --git a/src/libsystemd-network/sd-dhcp6-client.c 
 b/src/libsystemd-network/sd-dhcp6-client.c
 index 048b4ea..bdd9177 100644
 --- a/src/libsystemd-network/sd-dhcp6-client.c
 +++ b/src/libsystemd-network/sd-dhcp6-client.c
 @@ -48,6 +48,9 @@ struct sd_dhcp6_client {
  struct ether_addr mac_addr;
  icmp6_nd *ra;
  DHCP6IA ia_na;
 +be32_t transaction_id;
 +int fd;
 +sd_event_source *receive_message;
  usec_t retransmit_time;
  uint8_t retransmit_count;
  sd_event_source *timeout_resend;
 @@ -62,6 +65,24 @@ struct sd_dhcp6_client {
  } _packed_ duid;
  };
  
 +const char * dhcp6_message_type_table[_DHCP6_MESSAGE_MAX] = {
 +[DHCP6_SOLICIT] = SOLICIT,
 +[DHCP6_ADVERTISE] = ADVERTISE,
 +[DHCP6_REQUEST] = REQUEST,
 +[DHCP6_CONFIRM] = CONFIRM,
 +[DHCP6_RENEW] = RENEW,
 +[DHCP6_REBIND] = REBIND,
 +[DHCP6_REPLY] = REPLY,
 +[DHCP6_RELEASE] = RELEASE,
 +[DHCP6_DECLINE] = DECLINE,
 +[DHCP6_RECONFIGURE] = RECONFIGURE,
 +[DHCP6_INFORMATION_REQUEST] = INFORMATION-REQUEST,
 +[DHCP6_RELAY_FORW] = RELAY-FORW,
 +[DHCP6_RELAY_REPL] = RELAY-REPL,
 +};
 +
 +DEFINE_STRING_TABLE_LOOKUP(dhcp6_message_type, int);
 +
  int sd_dhcp6_client_set_callback(sd_dhcp6_client *client,
   sd_dhcp6_client_cb_t cb, void *userdata)
  {
 @@ -110,6 +131,15 @@ static int client_initialize(sd_dhcp6_client *client)
  {
  assert_return(client, -EINVAL);
  
 +client-receive_message =
 +sd_event_source_unref(client-receive_message);
 +

 +if (client-fd  0)
 +safe_close(client-fd);
 +client-fd = -1;
client-fd = safe_close(client-fd);

That's what safe_close is for :)

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

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-18 Thread Filipe Brandenburger 
On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
zbys...@in.waw.pl wrote:
 +if (client-fd  0)
 +safe_close(client-fd);
 +client-fd = -1;
 client-fd = safe_close(client-fd);

 That's what safe_close is for :)

And shouldn't the check be for client-fd = 0? Zero is a valid file descriptor.

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

Re: [systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-18 Thread Zbigniew Jędrzejewski-Szmek 
On Wed, Jun 18, 2014 at 07:05:35AM -0700, Filipe Brandenburger wrote:
 On Wed, Jun 18, 2014 at 6:58 AM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl wrote:
  +if (client-fd  0)
  +safe_close(client-fd);
  +client-fd = -1;
  client-fd = safe_close(client-fd);
 
  That's what safe_close is for :)
 
 And shouldn't the check be for client-fd = 0? Zero is a valid file 
 descriptor.
Yeah... but note that safe_close already has the fd = 0 check, so the
replacement line replaces the if too.

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

[systemd-devel] [PATCH 12/24] sd-dhcp6-client: Add DHCPv6 Solicit message creation and sending

2014-06-13 Thread Patrik Flykt 
Implement the initial functionality used for creating a DHCPv6 Solicit
message containing the needed options and send it to the DHCPv6
broadcast address. Increase the sent message count and ensure that
the Solicit Initial Retransmission Time is strictly greater than
the Solicitation IRT as described in RFC 3315, section 17.1.2.
---
 src/libsystemd-network/dhcp6-internal.h  |   4 ++
 src/libsystemd-network/sd-dhcp6-client.c | 111 +++
 2 files changed, 115 insertions(+)

diff --git a/src/libsystemd-network/dhcp6-internal.h 
b/src/libsystemd-network/dhcp6-internal.h
index 7a491fb..31f5bd2 100644
--- a/src/libsystemd-network/dhcp6-internal.h
+++ b/src/libsystemd-network/dhcp6-internal.h
@@ -27,6 +27,7 @@
 #include sparse-endian.h
 #include sd-event.h
 #include list.h
+#include macro.h
 
 typedef struct DHCP6Address DHCP6Address;
 
@@ -69,3 +70,6 @@ int dhcp6_option_parse(uint8_t **buf, size_t *buflen, 
uint16_t *optcode,
 int dhcp6_network_bind_udp_socket(int index, struct in6_addr *address);
 int dhcp6_network_send_udp_socket(int s, struct in6_addr *address,
   const void *packet, size_t len);
+
+const char *dhcp6_message_type_to_string(int s) _const_;
+int dhcp6_message_type_from_string(const char *s) _pure_;
diff --git a/src/libsystemd-network/sd-dhcp6-client.c 
b/src/libsystemd-network/sd-dhcp6-client.c
index 048b4ea..bdd9177 100644
--- a/src/libsystemd-network/sd-dhcp6-client.c
+++ b/src/libsystemd-network/sd-dhcp6-client.c
@@ -48,6 +48,9 @@ struct sd_dhcp6_client {
 struct ether_addr mac_addr;
 icmp6_nd *ra;
 DHCP6IA ia_na;
+be32_t transaction_id;
+int fd;
+sd_event_source *receive_message;
 usec_t retransmit_time;
 uint8_t retransmit_count;
 sd_event_source *timeout_resend;
@@ -62,6 +65,24 @@ struct sd_dhcp6_client {
 } _packed_ duid;
 };
 
+const char * dhcp6_message_type_table[_DHCP6_MESSAGE_MAX] = {
+[DHCP6_SOLICIT] = SOLICIT,
+[DHCP6_ADVERTISE] = ADVERTISE,
+[DHCP6_REQUEST] = REQUEST,
+[DHCP6_CONFIRM] = CONFIRM,
+[DHCP6_RENEW] = RENEW,
+[DHCP6_REBIND] = REBIND,
+[DHCP6_REPLY] = REPLY,
+[DHCP6_RELEASE] = RELEASE,
+[DHCP6_DECLINE] = DECLINE,
+[DHCP6_RECONFIGURE] = RECONFIGURE,
+[DHCP6_INFORMATION_REQUEST] = INFORMATION-REQUEST,
+[DHCP6_RELAY_FORW] = RELAY-FORW,
+[DHCP6_RELAY_REPL] = RELAY-REPL,
+};
+
+DEFINE_STRING_TABLE_LOOKUP(dhcp6_message_type, int);
+
 int sd_dhcp6_client_set_callback(sd_dhcp6_client *client,
  sd_dhcp6_client_cb_t cb, void *userdata)
 {
@@ -110,6 +131,15 @@ static int client_initialize(sd_dhcp6_client *client)
 {
 assert_return(client, -EINVAL);
 
+client-receive_message =
+sd_event_source_unref(client-receive_message);
+
+if (client-fd  0)
+safe_close(client-fd);
+client-fd = -1;
+
+client-transaction_id = random_u32()  0x00ff;
+
 client-ia_na.timeout_t1 =
 sd_event_source_unref(client-ia_na.timeout_t1);
 client-ia_na.timeout_t2 =
@@ -136,6 +166,55 @@ static sd_dhcp6_client *client_stop(sd_dhcp6_client 
*client, int error) {
 return client;
 }
 
+static int client_send_message(sd_dhcp6_client *client) {
+_cleanup_free_ DHCP6Message *message = NULL;
+struct in6_addr all_servers =
+IN6ADDR_ALL_DHCP6_RELAY_AGENTS_AND_SERVERS_INIT;
+size_t len, optlen = 512;
+uint8_t *opt;
+int r;
+
+len = sizeof(DHCP6Message) + optlen;
+
+message = malloc0(len);
+if (!message)
+return -ENOMEM;
+
+opt = (uint8_t *)(message + 1);
+
+message-transaction_id = client-transaction_id;
+
+switch(client-state) {
+case DHCP6_STATE_SOLICITATION:
+message-type = DHCP6_SOLICIT;
+
+r = dhcp6_option_append(opt, optlen, DHCP6_OPTION_CLIENTID,
+sizeof(client-duid), client-duid);
+if (r  0)
+return r;
+
+r = dhcp6_option_append_ia(opt, optlen, client-ia_na);
+if (r  0)
+return r;
+
+break;
+
+case DHCP6_STATE_STOPPED:
+case DHCP6_STATE_RS:
+return -EINVAL;
+}
+
+r = dhcp6_network_send_udp_socket(client-fd, all_servers, message,
+  len - optlen);
+if (r  0)
+return r;
+
+log_dhcp6_client(client, Sent %s,
+ dhcp6_message_type_to_string(message-type));
+
+return 0;
+}
+
 static int client_timeout_resend_expire(sd_event_source *s, uint64_t usec,
 void *userdata) {
 sd_dhcp6_client *client = userdata;
@@ -189,6 +268,11 @@ static int