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

Victorien Molle victorien.molle at wifirst.fr
Tue Feb 11 12:28:29 GMT 2020


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 +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