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

Simon Kelley simon at thekelleys.org.uk
Wed Feb 12 23:29:59 GMT 2020


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 +0000, 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 <victorien.molle at wifirst.fr>
>>> Signed-off-by: Florent Fourcot <florent.fourcot at wifirst.fr>
>>> ---
>>>  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 =
>>>  "    <method name=\"SetBogusPrivOption\">\n"
>>>  "      <arg name=\"boguspriv\" direction=\"in\" type=\"b\"/>\n"
>>>  "    </method>\n"
>>> +#ifdef HAVE_DHCP
>>>  "    <signal name=\"DhcpLeaseAdded\">\n"
>>>  "      <arg name=\"ipaddr\" type=\"s\"/>\n"
>>>  "      <arg name=\"hwaddr\" type=\"s\"/>\n"
>>> @@ -70,7 +71,13 @@ const char* introspection_xml_template =
>>>  "      <arg name=\"hwaddr\" type=\"s\"/>\n"
>>>  "      <arg name=\"hostname\" type=\"s\"/>\n"
>>>  "    </signal>\n"
>>> -#ifdef HAVE_DHCP
>>> +"    <signal name=\"DhcpLeaseNotification\">\n"
>>> +"      <arg name=\"notification_type\" type=\"i\"/>\n"
>>> +"      <arg name=\"ipaddr\" type=\"s\"/>\n"
>>> +"      <arg name=\"hwaddr\" type=\"s\"/>\n"
>>> +"      <arg name=\"hostname\" type=\"s\"/>\n"
>>> +"      <arg name=\"lease_info\" type=\"a{sv}\"/>\n"
>>> +"    </signal>\n"
>>>  "    <method name=\"AddDhcpLease\">\n"
>>>  "       <arg name=\"ipaddr\" type=\"s\"/>\n"
>>>  "       <arg name=\"hwaddr\" type=\"s\"/>\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 = "expiration", .fmt = "t", .dbus_type = DBUS_TYPE_UINT64, .value.u64 = expires},
>>> +                               {NULL, NULL, DBUS_TYPE_INVALID, 0}
>>> +                               };
>>> +
>>> +  if (!(message = dbus_message_new_signal(DNSMASQ_PATH, daemon->dbus_name, "DhcpLeaseNotification")))
>>> +    goto out;
>>> +
>>> +  dbus_message_iter_init_append(message, &iter);
>>> +  dbus_message_iter_append_basic(&iter, DBUS_TYPE_INT32, &act_type);
>>> +  dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &ipaddr);
>>> +  dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &hwaddr);
>>> +  dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &hostname);
>>> +  dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &array);
>>> +  for (info = infos; info->key; info++) {
>>> +    dbus_message_iter_open_container(&array, DBUS_TYPE_DICT_ENTRY, NULL, &dict);
>>> +    dbus_message_iter_append_basic(&dict, DBUS_TYPE_STRING, &info->key);
>>> +    dbus_message_iter_open_container(&dict, DBUS_TYPE_VARIANT, info->fmt, &variant);
>>> +    switch (info->dbus_type)
>>> +      {
>>> +      case DBUS_TYPE_STRING:
>>> +        dbus_message_iter_append_basic(&variant, info->dbus_type, &info->value.str);
>>> +        break;
>>> +
>>> +      case DBUS_TYPE_UINT64:
>>> +        dbus_message_iter_append_basic(&variant, info->dbus_type, &info->value.u64);
>>> +        break;
>>> +
>>> +      default:
>>> +        dbus_message_iter_abandon_container(&dict, &variant);
>>> +        dbus_message_iter_abandon_container_if_open(&array, &dict);
>>> +        dbus_message_iter_abandon_container_if_open(&iter, &array);
>>> +        dbus_message_unref(message);
>>> +        my_syslog(LOG_WARNING, _("Unknown D-Bus type specified for key '%s'. This will completely disable 'DhcpLeaseNotification' signal"),
>>> +                                 info->key);
>>> +        goto out;
>>> +      }
>>> +    dbus_message_iter_close_container(&dict, &variant);
>>> +    dbus_message_iter_close_container(&array, &dict);
>>> +  }
>>> +  dbus_message_iter_close_container(&iter, &array);
>>> +  dbus_connection_send(connection, message, NULL);
>>> +  dbus_message_unref(message);
>>> +
>>> +out:
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>> +  if (vendor_class && allocated)
>>> +    free(vendor_class);
>>> +#endif
>>> +}
>>> +
>>>  void emit_dbus_signal(int action, struct dhcp_lease *lease, char *hostname)
>>>  {
>>>    DBusConnection *connection = (DBusConnection *)daemon->dbus;
>>> @@ -878,6 +991,9 @@ void emit_dbus_signal(int action, struct dhcp_lease *lease, char *hostname)
>>>      dbus_connection_send(connection, message, NULL);
>>>    
>>>    dbus_message_unref(message);
>>> +
>>> +  /* Now emit a notification with more informations */
>>> +  emit_dhcplease_notification(connection, action, lease, daemon->addrbuff, mac, hostname);
>>>  }
>>>  #endif
>>>  
>>> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
>>> index 8e047fc..6b79685 100644
>>> --- a/src/dnsmasq.h
>>> +++ b/src/dnsmasq.h
>>> @@ -1415,7 +1415,7 @@ void lease_update_from_configs(void);
>>>  int do_script_run(time_t now);
>>>  void rerun_scripts(void);
>>>  void lease_find_interfaces(time_t now);
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>  void lease_add_extradata(struct dhcp_lease *lease, unsigned char *data, 
>>>  			 unsigned int len, int delim);
>>>  #endif
>>> diff --git a/src/lease.c b/src/lease.c
>>> index 081d90e..107b79a 100644
>>> --- a/src/lease.c
>>> +++ b/src/lease.c
>>> @@ -1158,7 +1158,7 @@ int do_script_run(time_t now)
>>>    return 0; /* nothing to do */
>>>  }
>>>  
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>  /* delim == -1 -> delim = 0, but embedded 0s, creating extra records, are OK. */
>>>  void lease_add_extradata(struct dhcp_lease *lease, unsigned char *data, unsigned int len, int delim)
>>>  {
>>> diff --git a/src/rfc2131.c b/src/rfc2131.c
>>> index 0058747..d4e9802 100644
>>> --- a/src/rfc2131.c
>>> +++ b/src/rfc2131.c
>>> @@ -21,7 +21,7 @@
>>>  #define option_len(opt) ((int)(((unsigned char *)(opt))[1]))
>>>  #define option_ptr(opt, i) ((void *)&(((unsigned char *)(opt))[2u+(unsigned int)(i)]))
>>>  
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>  static void add_extradata_opt(struct dhcp_lease *lease, unsigned char *opt);
>>>  #endif
>>>  
>>> @@ -97,7 +97,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
>>>    struct dhcp_opt *o;
>>>    unsigned char pxe_uuid[17];
>>>    unsigned char *oui = NULL, *serial = NULL;
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>    unsigned char *class = NULL;
>>>  #endif
>>>  
>>> @@ -163,7 +163,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
>>>  		  unsigned char *y = option_ptr(opt, offset + elen + 5);
>>>  		  oui = option_find1(x, y, 1, 1);
>>>  		  serial = option_find1(x, y, 2, 1);
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>  		  class = option_find1(x, y, 3, 1);		  
>>>  #endif
>>>  		  /* If TR069-id is present set the tag "cpewan-id" to facilitate echoing 
>>> @@ -1365,8 +1365,8 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
>>>  	      /* pick up INIT-REBOOT events. */
>>>  	      lease->flags |= LEASE_CHANGED;
>>>  
>>> -#ifdef HAVE_SCRIPT
>>> -	      if (daemon->lease_change_command)
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>> +	      if (daemon->lease_change_command || daemon->dbus)
>>>  		{
>>>  		  struct dhcp_netid *n;
>>>  		  
>>> @@ -1661,7 +1661,7 @@ static int sanitise(unsigned char *opt, char *buf)
>>>    return 1;
>>>  }
>>>  
>>> -#ifdef HAVE_SCRIPT
>>> +#if defined(HAVE_SCRIPT) || defined(HAVE_DBUS)
>>>  static void add_extradata_opt(struct dhcp_lease *lease, unsigned char *opt)
>>>  {
>>>    if (!opt)
>>>
>>
> 




More information about the Dnsmasq-discuss mailing list