Re: [Dnsmasq-discuss] [PATCH V2] Add new extensible D-Bus signal

2020-02-12 Thread Simon Kelley
On 11/02/2020 12:28, Victorien Molle wrote:
> Hi Simon, sorry for the late reply.
> 
> Here are my answers:
> 
> 1) This is because I prefer to work with integers and not strings.
> I personnaly prefer to declare enums for each action and then perform
> a boolean comparison instead of doing strings comparison.
> But I can use action strings if you prefer.

Definitely. You can't use those integers in the external DBus API unless
you define them, and having done that they can't be changed, so non-DBus
code is tied forever to the API. Make the notification type a string and
define it to be DhcpLeaseDeleted, DhcpLeaseAdded, or DhcpLeaseUpdated.

> 
> 2) 3) and 3a) I will look at it.
> 
> 4) Agree but I would like to be sure of one thing. At the start of
> DNSMasq, the start_time will be stored in the 'now' variable (line 240 of
> dnsmasq.c) and then in 'daemon->soa_sn' variable (line 253 of dnsmasq.c).
> Is it the right way to use daemon->soa_sn when calling the 'difftime' function
> outside of the 'main' function, or is there a better way to access to the 
> 'now'
> variable outside of the 'main' function?

daemon->soa_sn is not relevant to this. Don't use it. The now variable
gets updated each time the main loop is called, and passed as an
argument to any function that needs it. Just add time_t now as an
argument to emit_dbus_signal() and emit_dhcplease_notification().
emit_dbus_signal() is called from do_script_run() in lease.c which is
already passed the value of now as an argument.
> 
> 5) For now, I included data I need for my own usage but I can embed more
> data if necessary.

>From experience, if it's not done now, someone will want it in the
future. It's easier for everyone to try an pre-empt that whilst we can.



I know I'm in no position to complain about the time taken to get things
done, but I really am trying to tie up loose ends and get a 2.81 release
done, so it would be great to get a new patch fairly soon :) After all
this time it would be sad to be delaying waiting for this when 2.81 is
ready to go.

Cheers,

Simon.


> 
> Regards,
> Victorien
> 
> On Sun, Jan 26, 2020 at 11:52:35PM +, Simon Kelley wrote:
>> Sorry to keep pushing this back at you, but I looked in more detail and
>> there are things that are not yet right.
>>
>> 1) Why is the action the internal arbitrary integer? Wouldn't it be
>> better for this to be a string, deleted/added/updated or even the name
>> of the earlier signals; DhcpLeaseAdded etc which are in the action_str
>> variable already?
>>
>> 2) There is a few instances of code in dbus.c which have got wrapped with
>>
>> #if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>
>> which is pointless, since HAVE_DBUS must be defined otherwise the whole
>> file is omitted.
>>
>> 3) You're allocating a copy of the vendorclass but that's not necessary,
>> just past the pointer to the start of the extradata buffer.
>>
>> 3a) For an IPv6 lease, the vendorclass is different, see the code around
>> line 586 in src/helper.c and line 1783 in src/rfc3315.c, the extradata
>> buffer contains a (string) vendor-id first, and then some number of
>> vendor-class strings, the number is stored in lease->vendorclass_count.
>> Again, you don't need to copy these, just generate pointers into the
>> zero-delimited extradata buffer.
>>
>> 4) The lease expiration time is in unix epoch time, except sometimes it
>> isn't, if dnsmasq is compiled with   HAVE_BROKEN_RTC then it's in
>> seconds since last boot. The field name in the dictionary should at
>> least reflect this: but maybe it would be better to just provide the
>> time until lease expiration in seconds, that's always calculated the
>> same way: see helper.c line 788.
>>
>> 5) Question: there's lots of other data which is exported to the script,
>> as we now have an extensible way to export if via DBUS, should that data
>> be included in the DBus Dict?
>>
>>
>>
>> Cheers,
>>
>> Simon.
>>
>>
>>
>> On 23/01/2020 11:00, Victorien Molle wrote:
>>> For our usage, we need to have more informations sent over D-Bus such as 
>>> the interface
>>> name, the vendor class identifier and the lease expiration time.
>>>
>>> To achieve this, we add a new D-Bus signal "DhcpLeaseNotification" which 
>>> exports the
>>> requested informations as a dictionnary.
>>> It also has the advantage to be flexible if someone wants to add a new 
>>> entry in the
>>> future.
>>>
>>> Note: in order to get leases extradata be populated, we enabled this 
>>> feature if D-Bus
>>> is enabled in configuration file (this was enabled in the past only if a 
>>> script was
>>> ran on leases updates).
>>>
>>> Here is an example of the obtained result with a Python3 program:
>>>
>>> ''' Define our D-Bus callback '''
>>> def cb(action, ipaddr, hwaddr, hostname, info):
>>> print(f'Action: {action}')
>>> print(f'IP: {ipaddr}')
>>> print(f'Hostname: {hostname}')
>>> for k in info:
>>> print(f'{k}: {info.get(k)}')
>>>
>>> ''' Connect to signal 

Re: [Dnsmasq-discuss] [PATCH V2] Add new extensible D-Bus signal

2020-02-11 Thread Victorien Molle
Hi Simon, sorry for the late reply.

Here are my answers:

1) This is because I prefer to work with integers and not strings.
I personnaly prefer to declare enums for each action and then perform
a boolean comparison instead of doing strings comparison.
But I can use action strings if you prefer.

2) 3) and 3a) I will look at it.

4) Agree but I would like to be sure of one thing. At the start of
DNSMasq, the start_time will be stored in the 'now' variable (line 240 of
dnsmasq.c) and then in 'daemon->soa_sn' variable (line 253 of dnsmasq.c).
Is it the right way to use daemon->soa_sn when calling the 'difftime' function
outside of the 'main' function, or is there a better way to access to the 'now'
variable outside of the 'main' function?

5) For now, I included data I need for my own usage but I can embed more
data if necessary.

Regards,
Victorien

On Sun, Jan 26, 2020 at 11:52:35PM +, Simon Kelley wrote:
> Sorry to keep pushing this back at you, but I looked in more detail and
> there are things that are not yet right.
> 
> 1) Why is the action the internal arbitrary integer? Wouldn't it be
> better for this to be a string, deleted/added/updated or even the name
> of the earlier signals; DhcpLeaseAdded etc which are in the action_str
> variable already?
> 
> 2) There is a few instances of code in dbus.c which have got wrapped with
> 
> #if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
> 
> which is pointless, since HAVE_DBUS must be defined otherwise the whole
> file is omitted.
> 
> 3) You're allocating a copy of the vendorclass but that's not necessary,
> just past the pointer to the start of the extradata buffer.
> 
> 3a) For an IPv6 lease, the vendorclass is different, see the code around
> line 586 in src/helper.c and line 1783 in src/rfc3315.c, the extradata
> buffer contains a (string) vendor-id first, and then some number of
> vendor-class strings, the number is stored in lease->vendorclass_count.
> Again, you don't need to copy these, just generate pointers into the
> zero-delimited extradata buffer.
> 
> 4) The lease expiration time is in unix epoch time, except sometimes it
> isn't, if dnsmasq is compiled with   HAVE_BROKEN_RTC then it's in
> seconds since last boot. The field name in the dictionary should at
> least reflect this: but maybe it would be better to just provide the
> time until lease expiration in seconds, that's always calculated the
> same way: see helper.c line 788.
> 
> 5) Question: there's lots of other data which is exported to the script,
> as we now have an extensible way to export if via DBUS, should that data
> be included in the DBus Dict?
> 
> 
> 
> Cheers,
> 
> Simon.
> 
> 
> 
> On 23/01/2020 11:00, Victorien Molle wrote:
> > For our usage, we need to have more informations sent over D-Bus such as 
> > the interface
> > name, the vendor class identifier and the lease expiration time.
> > 
> > To achieve this, we add a new D-Bus signal "DhcpLeaseNotification" which 
> > exports the
> > requested informations as a dictionnary.
> > It also has the advantage to be flexible if someone wants to add a new 
> > entry in the
> > future.
> > 
> > Note: in order to get leases extradata be populated, we enabled this 
> > feature if D-Bus
> > is enabled in configuration file (this was enabled in the past only if a 
> > script was
> > ran on leases updates).
> > 
> > Here is an example of the obtained result with a Python3 program:
> > 
> > ''' Define our D-Bus callback '''
> > def cb(action, ipaddr, hwaddr, hostname, info):
> > print(f'Action: {action}')
> > print(f'IP: {ipaddr}')
> > print(f'Hostname: {hostname}')
> > for k in info:
> > print(f'{k}: {info.get(k)}')
> > 
> > ''' Connect to signal DhcpLeaseNotification on interface 
> > uk.org.thekelleys.dnsmasq '''
> > DNSMasq.listen('DhcpLeaseNotification', callback=cb)
> > 
> > ''' Run GLib loop '''
> > GLib.MainLoop().run()
> > 
> > ''' When DNSMasq receives a DHCP request, here is the result: '''
> > 
> > Action: 3
> > IP: 192.168.1.100
> > Hostname: Nucleus.nucle.us
> > interface: br-mgmt
> > vendor_class: LaGrosseBiche
> > expiration: 1575667431
> > 
> > Signed-off-by: Victorien Molle 
> > Signed-off-by: Florent Fourcot 
> > ---
> >  dbus/DBus-interface |   9 
> >  src/dbus.c  | 118 +++-
> >  src/dnsmasq.h   |   2 +-
> >  src/lease.c |   2 +-
> >  src/rfc2131.c   |  12 ++---
> >  5 files changed, 134 insertions(+), 9 deletions(-)
> > 
> > diff --git a/dbus/DBus-interface b/dbus/DBus-interface
> > index 954c5b9..ed42551 100644
> > --- a/dbus/DBus-interface
> > +++ b/dbus/DBus-interface
> > @@ -273,14 +273,23 @@ DhcpLeaseAdded
> >  ---
> >  
> >  This signal is emitted when a DHCP lease for a given IP address is created.
> > +This will also trigger the DhcpLeaseNotification signal.
> >  
> >  DhcpLeaseDeleted
> >  
> >  
> >  This signal is emitted when a DHCP lease for a given IP address is deleted.
> > 

Re: [Dnsmasq-discuss] [PATCH V2] Add new extensible D-Bus signal

2020-01-26 Thread Simon Kelley
Sorry to keep pushing this back at you, but I looked in more detail and
there are things that are not yet right.

1) Why is the action the internal arbitrary integer? Wouldn't it be
better for this to be a string, deleted/added/updated or even the name
of the earlier signals; DhcpLeaseAdded etc which are in the action_str
variable already?

2) There is a few instances of code in dbus.c which have got wrapped with

#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)

which is pointless, since HAVE_DBUS must be defined otherwise the whole
file is omitted.

3) You're allocating a copy of the vendorclass but that's not necessary,
just past the pointer to the start of the extradata buffer.

3a) For an IPv6 lease, the vendorclass is different, see the code around
line 586 in src/helper.c and line 1783 in src/rfc3315.c, the extradata
buffer contains a (string) vendor-id first, and then some number of
vendor-class strings, the number is stored in lease->vendorclass_count.
Again, you don't need to copy these, just generate pointers into the
zero-delimited extradata buffer.

4) The lease expiration time is in unix epoch time, except sometimes it
isn't, if dnsmasq is compiled with   HAVE_BROKEN_RTC then it's in
seconds since last boot. The field name in the dictionary should at
least reflect this: but maybe it would be better to just provide the
time until lease expiration in seconds, that's always calculated the
same way: see helper.c line 788.

5) Question: there's lots of other data which is exported to the script,
as we now have an extensible way to export if via DBUS, should that data
be included in the DBus Dict?



Cheers,

Simon.



On 23/01/2020 11:00, Victorien Molle wrote:
> For our usage, we need to have more informations sent over D-Bus such as the 
> interface
> name, the vendor class identifier and the lease expiration time.
> 
> To achieve this, we add a new D-Bus signal "DhcpLeaseNotification" which 
> exports the
> requested informations as a dictionnary.
> It also has the advantage to be flexible if someone wants to add a new entry 
> in the
> future.
> 
> Note: in order to get leases extradata be populated, we enabled this feature 
> if D-Bus
> is enabled in configuration file (this was enabled in the past only if a 
> script was
> ran on leases updates).
> 
> Here is an example of the obtained result with a Python3 program:
> 
> ''' Define our D-Bus callback '''
> def cb(action, ipaddr, hwaddr, hostname, info):
> print(f'Action: {action}')
> print(f'IP: {ipaddr}')
> print(f'Hostname: {hostname}')
> for k in info:
> print(f'{k}: {info.get(k)}')
> 
> ''' Connect to signal DhcpLeaseNotification on interface 
> uk.org.thekelleys.dnsmasq '''
> DNSMasq.listen('DhcpLeaseNotification', callback=cb)
> 
> ''' Run GLib loop '''
> GLib.MainLoop().run()
> 
> ''' When DNSMasq receives a DHCP request, here is the result: '''
> 
> Action: 3
> IP: 192.168.1.100
> Hostname: Nucleus.nucle.us
> interface: br-mgmt
> vendor_class: LaGrosseBiche
> expiration: 1575667431
> 
> Signed-off-by: Victorien Molle 
> Signed-off-by: Florent Fourcot 
> ---
>  dbus/DBus-interface |   9 
>  src/dbus.c  | 118 +++-
>  src/dnsmasq.h   |   2 +-
>  src/lease.c |   2 +-
>  src/rfc2131.c   |  12 ++---
>  5 files changed, 134 insertions(+), 9 deletions(-)
> 
> diff --git a/dbus/DBus-interface b/dbus/DBus-interface
> index 954c5b9..ed42551 100644
> --- a/dbus/DBus-interface
> +++ b/dbus/DBus-interface
> @@ -273,14 +273,23 @@ DhcpLeaseAdded
>  ---
>  
>  This signal is emitted when a DHCP lease for a given IP address is created.
> +This will also trigger the DhcpLeaseNotification signal.
>  
>  DhcpLeaseDeleted
>  
>  
>  This signal is emitted when a DHCP lease for a given IP address is deleted.
> +This will also trigger the DhcpLeaseNotification signal.
>  
>  DhcpLeaseUpdated
>  
>  
>  This signal is emitted when a DHCP lease for a given IP address is updated.
> +This will also trigger the DhcpLeaseNotification signal.
> +
> +DhcpLeaseNotification
> +-
> +
> +This signal is emitted when a DHCP lease action is triggered. It exports,
> +as a dictionnary, more informations than the other signals.
>   
> diff --git a/src/dbus.c b/src/dbus.c
> index c0ce903..c906e11 100644
> --- a/src/dbus.c
> +++ b/src/dbus.c
> @@ -55,6 +55,7 @@ const char* introspection_xml_template =
>  "\n"
>  "  \n"
>  "\n"
> +#ifdef HAVE_DHCP
>  "\n"
>  "  \n"
>  "  \n"
> @@ -70,7 +71,13 @@ const char* introspection_xml_template =
>  "  \n"
>  "  \n"
>  "\n"
> -#ifdef HAVE_DHCP
> +"\n"
> +"  \n"
> +"  \n"
> +"  \n"
> +"  \n"
> +"  \n"
> +"\n"
>  "\n"
>  "   \n"
>  "   \n"
> @@ -98,6 +105,12 @@ struct watch {
>struct watch *next;
>  };
>  
> +struct lease_info {
> +  char *key;
> +  char *fmt;
> +  char dbus_type;
> +  DBusBasicValue value;
> 

[Dnsmasq-discuss] [PATCH V2] Add new extensible D-Bus signal

2020-01-23 Thread Victorien Molle
For our usage, we need to have more informations sent over D-Bus such as the 
interface
name, the vendor class identifier and the lease expiration time.

To achieve this, we add a new D-Bus signal "DhcpLeaseNotification" which 
exports the
requested informations as a dictionnary.
It also has the advantage to be flexible if someone wants to add a new entry in 
the
future.

Note: in order to get leases extradata be populated, we enabled this feature if 
D-Bus
is enabled in configuration file (this was enabled in the past only if a script 
was
ran on leases updates).

Here is an example of the obtained result with a Python3 program:

''' Define our D-Bus callback '''
def cb(action, ipaddr, hwaddr, hostname, info):
print(f'Action: {action}')
print(f'IP: {ipaddr}')
print(f'Hostname: {hostname}')
for k in info:
print(f'{k}: {info.get(k)}')

''' Connect to signal DhcpLeaseNotification on interface 
uk.org.thekelleys.dnsmasq '''
DNSMasq.listen('DhcpLeaseNotification', callback=cb)

''' Run GLib loop '''
GLib.MainLoop().run()

''' When DNSMasq receives a DHCP request, here is the result: '''

Action: 3
IP: 192.168.1.100
Hostname: Nucleus.nucle.us
interface: br-mgmt
vendor_class: LaGrosseBiche
expiration: 1575667431

Signed-off-by: Victorien Molle 
Signed-off-by: Florent Fourcot 
---
 dbus/DBus-interface |   9 
 src/dbus.c  | 118 +++-
 src/dnsmasq.h   |   2 +-
 src/lease.c |   2 +-
 src/rfc2131.c   |  12 ++---
 5 files changed, 134 insertions(+), 9 deletions(-)

diff --git a/dbus/DBus-interface b/dbus/DBus-interface
index 954c5b9..ed42551 100644
--- a/dbus/DBus-interface
+++ b/dbus/DBus-interface
@@ -273,14 +273,23 @@ DhcpLeaseAdded
 ---
 
 This signal is emitted when a DHCP lease for a given IP address is created.
+This will also trigger the DhcpLeaseNotification signal.
 
 DhcpLeaseDeleted
 
 
 This signal is emitted when a DHCP lease for a given IP address is deleted.
+This will also trigger the DhcpLeaseNotification signal.
 
 DhcpLeaseUpdated
 
 
 This signal is emitted when a DHCP lease for a given IP address is updated.
+This will also trigger the DhcpLeaseNotification signal.
+
+DhcpLeaseNotification
+-
+
+This signal is emitted when a DHCP lease action is triggered. It exports,
+as a dictionnary, more informations than the other signals.
  
diff --git a/src/dbus.c b/src/dbus.c
index c0ce903..c906e11 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -55,6 +55,7 @@ const char* introspection_xml_template =
 "\n"
 "  \n"
 "\n"
+#ifdef HAVE_DHCP
 "\n"
 "  \n"
 "  \n"
@@ -70,7 +71,13 @@ const char* introspection_xml_template =
 "  \n"
 "  \n"
 "\n"
-#ifdef HAVE_DHCP
+"\n"
+"  \n"
+"  \n"
+"  \n"
+"  \n"
+"  \n"
+"\n"
 "\n"
 "   \n"
 "   \n"
@@ -98,6 +105,12 @@ struct watch {
   struct watch *next;
 };
 
+struct lease_info {
+  char *key;
+  char *fmt;
+  char dbus_type;
+  DBusBasicValue value;
+};
 
 static dbus_bool_t add_watch(DBusWatch *watch, void *data)
 {
@@ -828,6 +841,106 @@ void check_dbus_listeners()
 }
 
 #ifdef HAVE_DHCP
+/*
+  As this function is called by emit_dbus_signal, we already have access to 
ipaddr, hwaddr and hostname attributes
+  NOTE: connection attribute is currently not NULL as it is verified by 
emit_dbus_signal
+*/
+void emit_dhcplease_notification(DBusConnection *connection, int action, 
struct dhcp_lease *lease, char *ipaddr, char *hwaddr, char *hostname)
+{
+  DBusMessageIter array, dict, iter, variant;
+  dbus_uint64_t expires = lease->expires;
+  unsigned char *buf = lease->extradata;
+  dbus_int32_t act_type = action;
+  char interface[IF_NAMESIZE];
+  DBusMessage* message = NULL;
+  struct lease_info *info;
+  int fd = daemon->dhcpfd;
+#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
+  char *vendor_class;
+  int allocated = 0;
+#endif
+
+#ifdef HAVE_DHCP6
+  if (!daemon->dhcp)
+fd = daemon->dhcp6fd;
+#endif
+
+  /* Get interface name */
+  if (!indextoname(fd, lease->last_interface, interface))
+interface[0] = 0;
+
+#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
+  vendor_class = "";
+  /* As defined in rfc2131.c, the first data is the vendor class even if it is 
empty */
+  if (buf && *buf != 0)
+{
+  vendor_class = (char*)whine_malloc(lease->extradata_len + 1);
+  if (vendor_class)
+{
+  allocated = 1;
+  strncpy(vendor_class, (const char*)buf, (lease->extradata_len));
+}
+  else
+{
+  vendor_class = "";
+}
+}
+#endif
+
+  struct lease_info infos[] = {{.key = "interface", .fmt = "s", .dbus_type = 
DBUS_TYPE_STRING, .value.str = interface},
+#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
+   {.key = "vendor_class", .fmt = "s", .dbus_type 
= DBUS_TYPE_STRING, .value.str = vendor_class},
+#endif
+   {.key =