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

Simon Kelley simon at thekelleys.org.uk
Sun Jan 26 23:52:35 GMT 2020


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