Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-07 Thread Dan Winship 
On 04/04/2014 05:46 PM, Thomas Haller wrote:
 On Fri, 2014-04-04 at 16:24 -0500, Dan Williams wrote:
 On Fri, 2014-04-04 at 23:02 +0200, Thomas Haller wrote:
 On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote:
 Is the only reason for the #define of the common fields so that you
 don't have to do another level of indirection?  It looks somewhat ugly
 and my personal preference would be to just declare the base struct in
 the functions you want to use it in and up-cast if you need the v4 or v6
 version...  kinda like we do with objects.  So I certainly agree with
 the principle, but lets see what other people say about the
 implementation...

 I see, but the disadvantage is, that I would have to fixup *many*
 occurrences in existing code. Also, it is more typing.

But many of those existing occurrences involve separate IPv4 and IPv6
functions / codepaths where the actual behavior is exactly the same and
ought to be merged into a single NMPlatformIPAddress codepath anyway...

-- Dan

___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-07 Thread Thomas Haller 
On Mon, 2014-04-07 at 14:10 -0400, Dan Winship wrote:
 On 04/04/2014 05:46 PM, Thomas Haller wrote:
  On Fri, 2014-04-04 at 16:24 -0500, Dan Williams wrote:
  On Fri, 2014-04-04 at 23:02 +0200, Thomas Haller wrote:
  On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote:
  Is the only reason for the #define of the common fields so that you
  don't have to do another level of indirection?  It looks somewhat ugly
  and my personal preference would be to just declare the base struct in
  the functions you want to use it in and up-cast if you need the v4 or v6
  version...  kinda like we do with objects.  So I certainly agree with
  the principle, but lets see what other people say about the
  implementation...
 
  I see, but the disadvantage is, that I would have to fixup *many*
  occurrences in existing code. Also, it is more typing.
 
 But many of those existing occurrences involve separate IPv4 and IPv6
 functions / codepaths where the actual behavior is exactly the same and
 ought to be merged into a single NMPlatformIPAddress codepath anyway...
 
 -- Dan


I would say that many occurrences are no candidates for refactoring:

  $ git grep '\(-\|\.\)\lifetime\'

(which does not mean, that it isn't useful in several cases!)



I think, there are several reasons for the *_COMMON define:



* it would need more typing for no benefit:

-  addr.lifetime = lifetime;
+  addr.common.lifetime = lifetime;
or
+  ((NMPlatformIPAddress *) addr)-lifetime = lifetime;




* I have another version of the patch, that combines NMPlatformLink,
NMPlatform*Address and NMPlatform*Route with:

+#define __NMPlatformObject_COMMON \
+int ifindex; \
+;

that would mean:

-   addr.ifindex = ifindex;
-   addr.lifetime = lifetime;
+   addr.common_addr.common.ifindex = ifindex;
+   addr.common_addr.lifetime = lifetime;
or
+   ((NMPlatformObject *) addr)-ifindex = ifindex;
+   ((NMPlatformIPAddress *) addr)-lifetime = lifetime;




* I don't want to touch existing/working code now (even if it's
trivially fixing compile errors).



Thomas



PS: yes, I have an actual usecase for this patch


signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-04 Thread Dan Williams 
On Thu, 2014-04-03 at 14:23 +0200, Thomas Haller wrote:
 Especially the calculation of timestamps is identicall for addresses.
 By creating a base struct, we can use the same code for that, because
 NMPlatformIP4Address and NMPlatformIP6Address can now both be treated as
 NMPlatformIPAddress (and the same for routes).

Is the only reason for the #define of the common fields so that you
don't have to do another level of indirection?  It looks somewhat ugly
and my personal preference would be to just declare the base struct in
the functions you want to use it in and up-cast if you need the v4 or v6
version...  kinda like we do with objects.  So I certainly agree with
the principle, but lets see what other people say about the
implementation...

Dan

 Signed-off-by: Thomas Haller thal...@redhat.com
 ---
  I think introducing this base class could help us to put similar
  code for IPv4 and IPv6 types into a function. Especially handling of
  the timestamps/expiration.
 
  src/platform/nm-platform.h | 50 
 +-
  1 file changed, 32 insertions(+), 18 deletions(-)
 
 diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
 index 356af1f..dd56977 100644
 --- a/src/platform/nm-platform.h
 +++ b/src/platform/nm-platform.h
 @@ -143,19 +143,32 @@ typedef enum {
   NM_PLATFORM_SOURCE_USER,
  } NMPlatformSource;
  
 +#define __NMPlatformIPAddress_COMMON \
 + int ifindex; \
 + NMPlatformSource source; \
 + guint32 timestamp;  /* nm_utils_get_monotonic_timestamp_s() */ \
 + guint32 lifetime;   /* seconds */ \
 + guint32 preferred;  /* seconds */ \
 + ;
 +
 +/**
 + * NMPlatformIPAddress:
 + *
 + * Common parts of NMPlatformIP4Address and NMPlatformIP6Address.
 + **/
 +typedef struct {
 + __NMPlatformIPAddress_COMMON
 +} NMPlatformIPAddress;
 +
  /**
   * NMPlatformIP4Address:
   * @timestamp: timestamp as returned by nm_utils_get_monotonic_timestamp_s()
   **/
  typedef struct {
 - int ifindex;
 - NMPlatformSource source;
 + __NMPlatformIPAddress_COMMON
   in_addr_t address;
   in_addr_t peer_address;  /* PTP peer address */
   int plen;
 - guint32 timestamp;
 - guint32 lifetime;   /* seconds */
 - guint32 preferred;  /* seconds */
   char label[IFNAMSIZ];
  } NMPlatformIP4Address;
  
 @@ -164,37 +177,38 @@ typedef struct {
   * @timestamp: timestamp as returned by nm_utils_get_monotonic_timestamp_s()
   **/
  typedef struct {
 - int ifindex;
 - NMPlatformSource source;
 + __NMPlatformIPAddress_COMMON
   struct in6_addr address;
   struct in6_addr peer_address;
   int plen;
 - guint32 timestamp;  /* seconds */
 - guint32 lifetime;   /* seconds */
 - guint32 preferred;
   guint flags; /* ifa_flags from linux/if_addr.h, field type unsigned 
 int is as used in rtnl_addr_get_flags. */
  } NMPlatformIP6Address;
  
  #define NM_PLATFORM_ROUTE_METRIC_DEFAULT 1024
  
 +#define __NMPlatformIPRoute_COMMON \
 + int ifindex; \
 + NMPlatformSource source; \
 + guint metric; \
 + guint mss; \
 + ;
 +
  typedef struct {
 - int ifindex;
 - NMPlatformSource source;
 + __NMPlatformIPRoute_COMMON
 +} NMPlatformIPRoute;
 +
 +typedef struct {
 + __NMPlatformIPRoute_COMMON
   in_addr_t network;
   int plen;
   in_addr_t gateway;
 - guint metric;
 - guint mss;
  } NMPlatformIP4Route;
  
  typedef struct {
 - int ifindex;
 - NMPlatformSource source;
 + __NMPlatformIPRoute_COMMON
   struct in6_addr network;
   int plen;
   struct in6_addr gateway;
 - guint metric;
 - guint mss;
  } NMPlatformIP6Route;
  
  typedef struct {


___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-04 Thread Thomas Haller 
On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote:
 On Thu, 2014-04-03 at 14:23 +0200, Thomas Haller wrote:
  Especially the calculation of timestamps is identicall for addresses.
  By creating a base struct, we can use the same code for that, because
  NMPlatformIP4Address and NMPlatformIP6Address can now both be treated as
  NMPlatformIPAddress (and the same for routes).
 
 Is the only reason for the #define of the common fields so that you
 don't have to do another level of indirection?  It looks somewhat ugly
 and my personal preference would be to just declare the base struct in
 the functions you want to use it in and up-cast if you need the v4 or v6
 version...  kinda like we do with objects.  So I certainly agree with
 the principle, but lets see what other people say about the
 implementation...


Hi Dan,

could you elaborate an what would be your preference? I don't
understand.

Thomas


signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-04 Thread Dan Williams 
On Fri, 2014-04-04 at 23:02 +0200, Thomas Haller wrote:
 On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote:
  On Thu, 2014-04-03 at 14:23 +0200, Thomas Haller wrote:
   Especially the calculation of timestamps is identicall for addresses.
   By creating a base struct, we can use the same code for that, because
   NMPlatformIP4Address and NMPlatformIP6Address can now both be treated as
   NMPlatformIPAddress (and the same for routes).
  
  Is the only reason for the #define of the common fields so that you
  don't have to do another level of indirection?  It looks somewhat ugly
  and my personal preference would be to just declare the base struct in
  the functions you want to use it in and up-cast if you need the v4 or v6
  version...  kinda like we do with objects.  So I certainly agree with
  the principle, but lets see what other people say about the
  implementation...
 
 
 Hi Dan,
 
 could you elaborate an what would be your preference? I don't
 understand.

Sorry :)  I mean that I'm not wild about the #define of the common
fields,  but I'm OK with it if others think its fine.  Typically it be
something more like this:

typedef struct {
   NMPlatformIPAddress p;
   ipv4 stuff
} NMPlatformIP4Address;

typedef struct {
   NMPlatformIPAddress p;
   ipv6 stuff
} NMPlatformIP6Address;

void option1 (NMPlatformIP4Address *addr)
{
NMPlatformIPAddress *parent = (NMPlatformIP4Address *) addr;
addr-address = xxx;
}

void option2 (NMPlatformIP4Address *addr)
{
addr-p.address = xxx;
}

which is more in line with kernel/glibc style I think.  It does mean a
bit more typing though.

Dan

___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH 1/1] platform: extract common fields of IPv4/IPv6 addresses and routes to base struct

2014-04-04 Thread Thomas Haller 
On Fri, 2014-04-04 at 16:24 -0500, Dan Williams wrote:
 On Fri, 2014-04-04 at 23:02 +0200, Thomas Haller wrote:
  On Fri, 2014-04-04 at 15:13 -0500, Dan Williams wrote:
   On Thu, 2014-04-03 at 14:23 +0200, Thomas Haller wrote:
Especially the calculation of timestamps is identicall for addresses.
By creating a base struct, we can use the same code for that, because
NMPlatformIP4Address and NMPlatformIP6Address can now both be treated as
NMPlatformIPAddress (and the same for routes).
   
   Is the only reason for the #define of the common fields so that you
   don't have to do another level of indirection?  It looks somewhat ugly
   and my personal preference would be to just declare the base struct in
   the functions you want to use it in and up-cast if you need the v4 or v6
   version...  kinda like we do with objects.  So I certainly agree with
   the principle, but lets see what other people say about the
   implementation...
  
  
  Hi Dan,
  
  could you elaborate an what would be your preference? I don't
  understand.
 
 Sorry :)  I mean that I'm not wild about the #define of the common
 fields,  but I'm OK with it if others think its fine.  Typically it be
 something more like this:
 
 typedef struct {
NMPlatformIPAddress p;
ipv4 stuff
 } NMPlatformIP4Address;
 
 typedef struct {
NMPlatformIPAddress p;
ipv6 stuff
 } NMPlatformIP6Address;
 
 void option1 (NMPlatformIP4Address *addr)
 {
 NMPlatformIPAddress *parent = (NMPlatformIP4Address *) addr;
 addr-address = xxx;
 }
 
 void option2 (NMPlatformIP4Address *addr)
 {
 addr-p.address = xxx;
 }
 
 which is more in line with kernel/glibc style I think.  It does mean a
 bit more typing though.
 
 Dan
 


I see, but the disadvantage is, that I would have to fixup *many*
occurrences in existing code. Also, it is more typing.

This way is also how libnl3 does it with the NLHDR_COMMON define (just
to compete with your glib role model :) )


Thomas


signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list