This all looks sensible, with one exception: the logging in
set_ubus_listeners() and check_ubus_listeners() and associated with the
calls to check_ubus_listeners can potentially massively span the logs -
a long lived error will log multiple lines every time dnsmasq spins its
event loop. It would be good to have rate limiting there. Set a flag
after logging the error which inhibits further errors, clear it once a
normal situation is restored.


Cheers,

Simon.



On 25/03/2019 12:12, Jan Willem Janssen wrote:
> Improve the UBus support in DNSMASQ:
> 
> - Aligned the handling of UBus connections with the DBus code as it makes it 
> a bit easier
> to comprehend and allow for automatic reconnects when the connection to UBus 
> drops;
> - make sure that DNSMASQ can connect to UBus when running as another user 
> than root;
> - added status checks and logging to the various UBus calls to aid debugging 
> from an
> enduser point of view;
> - show the (lack of) support for UBus in the configuration string.
> 
> ---
>  src/config.h  |   4 ++
>  src/dnsmasq.c |  38 ++++++++++++++++-
>  src/dnsmasq.h |   6 +++
>  src/ubus.c    | 114 ++++++++++++++++++++++++++++++++++++++++----------
>  4 files changed, 139 insertions(+), 23 deletions(-)
> 
> diff --git a/src/config.h b/src/config.h
> index 203d69e..99b22eb 100644
> --- a/src/config.h
> +++ b/src/config.h
> @@ -362,6 +362,10 @@ static char *compile_opts =
>  "no-"
>  #endif
>  "DBus "
> +#ifndef HAVE_UBUS
> +"no-"
> +#endif
> +"UBus "
>  #ifndef LOCALEDIR
>  "no-"
>  #endif
> diff --git a/src/dnsmasq.c b/src/dnsmasq.c
> index 3f3edbd..5d89aa1 100644
> --- a/src/dnsmasq.c
> +++ b/src/dnsmasq.c
> @@ -420,6 +420,18 @@ int main (int argc, char **argv)
>    die(_("DBus not available: set HAVE_DBUS in src/config.h"), NULL, 
> EC_BADCONF);
>  #endif
>  
> +  if (option_bool(OPT_UBUS))
> +#ifdef HAVE_UBUS
> +    {
> +      const char *err;
> +      daemon->ubus = NULL;
> +      if ((err = ubus_init()))
> +        my_syslog(LOG_WARNING, _("UBus init failed: %s"), (char *)err, 
> EC_MISC);
> +    }
> +#else
> +  die(_("UBus not available: set HAVE_UBUS in src/config.h"), NULL, 
> EC_BADCONF);
> +#endif
> +
>    if (daemon->port != 0)
>      pre_allocate_sfds();
>  
> @@ -812,6 +824,16 @@ int main (int argc, char **argv)
>      }
>  #endif
>  
> +#ifdef HAVE_UBUS
> +  if (option_bool(OPT_UBUS))
> +    {
> +      if (daemon->ubus)
> +        my_syslog(LOG_INFO, _("UBus support enabled: connected to system 
> bus"));
> +      else
> +        my_syslog(LOG_INFO, _("UBus support enabled: bus connection 
> pending"));
> +    }
> +#endif
> +
>  #ifdef HAVE_DNSSEC
>    if (option_bool(OPT_DNSSEC_VALID))
>      {
> @@ -1000,7 +1022,7 @@ int main (int argc, char **argv)
>  
>  #ifdef HAVE_UBUS
>        if (option_bool(OPT_UBUS))
> -         set_ubus_listeners();
> +        set_ubus_listeners();
>  #endif
>           
>  #ifdef HAVE_DHCP
> @@ -1135,7 +1157,19 @@ int main (int argc, char **argv)
>  
>  #ifdef HAVE_UBUS
>        if (option_bool(OPT_UBUS))
> -        check_ubus_listeners();
> +        {
> +          /* if we didn't create a UBus connection, retry now. */
> +          if (!daemon->ubus)
> +            {
> +              const char *err;
> +              if ((err = ubus_init()))
> +                my_syslog(LOG_WARNING, _("UBus problem: %s"), err);
> +              if (daemon->ubus)
> +                my_syslog(LOG_INFO, _("Connected to system UBus"));
> +            }
> +
> +          check_ubus_listeners();
> +        }
>  #endif
>  
>        check_dns_listeners(now);
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 0e409b4..f5e154e 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -1119,6 +1119,11 @@ extern struct daemon {
>  #ifdef HAVE_DBUS
>    struct watch *watches;
>  #endif
> +  /* UBus stuff */
> +#ifdef HAVE_UBUS
> +  /* void * here to avoid depending on ubus headers outside ubus.c */
> +  void *ubus;
> +#endif
>  
>    /* TFTP stuff */
>    struct tftp_transfer *tftp_trans, *tftp_done_trans;
> @@ -1456,6 +1461,7 @@ void emit_dbus_signal(int action, struct dhcp_lease 
> *lease, char
> *hostname);
>  
>  /* ubus.c */
>  #ifdef HAVE_UBUS
> +const char *ubus_init(void);
>  void set_ubus_listeners(void);
>  void check_ubus_listeners(void);
>  void ubus_event_bcast(const char *type, const char *mac, const char *ip, 
> const char
> *name, const char *interface);
> diff --git a/src/ubus.c b/src/ubus.c
> index 703a60d..c58c231 100644
> --- a/src/ubus.c
> +++ b/src/ubus.c
> @@ -20,29 +20,94 @@
>  
>  #include <libubus.h>
>  
> -static struct ubus_context *ubus = NULL;
>  static struct blob_buf b;
> +static int notify;
>  
>  static int ubus_handle_metrics(struct ubus_context *ctx, struct ubus_object 
> *obj,
>                                struct ubus_request_data *req, const char 
> *method,
>                                struct blob_attr *msg);
> -static struct ubus_method ubus_object_methods[] = {
> -  {.name = "metrics", .handler = ubus_handle_metrics},
> +
> +static void ubus_subscribe_cb(struct ubus_context *ctx, struct ubus_object 
> *obj);
> +
> +static const struct ubus_method ubus_object_methods[] = {
> +  UBUS_METHOD_NOARG("metrics", ubus_handle_metrics),
>  };
>  
> -static struct ubus_object_type ubus_object_type = UBUS_OBJECT_TYPE("dnsmasq",
> ubus_object_methods);
> +static struct ubus_object_type ubus_object_type =
> +  UBUS_OBJECT_TYPE("dnsmasq", ubus_object_methods);
>  
>  static struct ubus_object ubus_object = {
>    .name = "dnsmasq",
>    .type = &ubus_object_type,
>    .methods = ubus_object_methods,
>    .n_methods = ARRAY_SIZE(ubus_object_methods),
> +  .subscribe_cb = ubus_subscribe_cb,
>  };
>  
> +static void ubus_subscribe_cb(struct ubus_context *ctx, struct ubus_object 
> *obj)
> +{
> +  (void)ctx;
> +
> +  my_syslog(LOG_DEBUG, _("UBus subscription callback: %s subscriber(s)"), 
> obj-
>> has_subscribers ? "1" : "0");
> +  notify = obj->has_subscribers;
> +}
> +
> +void ubus_destroy(struct ubus_context *ubus)
> +{
> +  // Forces re-initialization when we're reusing the same definitions later 
> on.
> +  ubus_object.id = 0;
> +  ubus_object_type.id = 0;
> +
> +  ubus_free(ubus);
> +  daemon->ubus = NULL;
> +}
> +
> +static void ubus_disconnect_cb(struct ubus_context *ubus)
> +{
> +  int ret;
> +
> +  ret = ubus_reconnect(ubus, NULL);
> +  if (ret)
> +    {
> +      my_syslog(LOG_ERR, _("Failed to reconnect to UBus: %s"), 
> ubus_strerror(ret));
> +
> +      ubus_destroy(ubus);
> +    }
> +}
> +
> +const char *ubus_init()
> +{
> +  struct ubus_context *ubus = NULL;
> +  int ret;
> +
> +  ubus = ubus_connect(NULL);
> +  if (!ubus)
> +    {
> +      ret = UBUS_STATUS_CONNECTION_FAILED;
> +      return ubus_strerror(ret);
> +    }
> +
> +  ubus->connection_lost = ubus_disconnect_cb;
> +
> +  ret = ubus_add_object(ubus, &ubus_object);
> +  if (ret)
> +    {
> +      return ubus_strerror(ret);
> +    }
> +
> +  daemon->ubus = ubus;
> +
> +  return NULL;
> +}
> +
>  void set_ubus_listeners()
>  {
> +  struct ubus_context *ubus = (struct ubus_context *)daemon->ubus;
>    if (!ubus)
> -    return;
> +    {
> +      my_syslog(LOG_ERR, _("Cannot set UBus listeners: no connection"));
> +      return;
> +    }
>  
>    poll_listen(ubus->sock.fd, POLLIN);
>    poll_listen(ubus->sock.fd, POLLERR);
> @@ -51,46 +116,51 @@ void set_ubus_listeners()
>  
>  void check_ubus_listeners()
>  {
> +  struct ubus_context *ubus = (struct ubus_context *)daemon->ubus;
>    if (!ubus)
>      {
> -      ubus = ubus_connect(NULL);
> -      if (!ubus)
> -       return;
> -      ubus_add_object(ubus, &ubus_object);
> +      my_syslog(LOG_ERR, _("Cannot poll UBus listeners: no connection"));
> +      return;
>      }
>    
>    if (poll_check(ubus->sock.fd, POLLIN))
>      ubus_handle_event(ubus);
>    
> -  if (poll_check(ubus->sock.fd, POLLHUP))
> +  if (poll_check(ubus->sock.fd, POLLHUP | POLLERR))
>      {
> -      ubus_free(ubus);
> -      ubus = NULL;
> +      my_syslog(LOG_INFO, _("Disconnecting from UBus"));
> +
> +      ubus_destroy(ubus);
>      }
>  }
>  
> -
>  static int ubus_handle_metrics(struct ubus_context *ctx, struct ubus_object 
> *obj,
>                                struct ubus_request_data *req, const char 
> *method,
>                                struct blob_attr *msg)
>  {
>    int i;
> -  blob_buf_init(&b, 0);
>  
> -  for(i=0; i < __METRIC_MAX; i++)
> +  (void)obj;
> +  (void)method;
> +  (void)msg;
> +
> +  blob_buf_init(&b, BLOBMSG_TYPE_TABLE);
> +
> +  for (i=0; i < __METRIC_MAX; i++)
>      blobmsg_add_u32(&b, get_metric_name(i), daemon->metrics[i]);
>    
> -  ubus_send_reply(ctx, req, b.head);
> -  
> -  return 0;
> +  return ubus_send_reply(ctx, req, b.head);
>  }
>  
>  void ubus_event_bcast(const char *type, const char *mac, const char *ip, 
> const char
> *name, const char *interface)
>  {
> -  if (!ubus || !ubus_object.has_subscribers)
> +  struct ubus_context *ubus = (struct ubus_context *)daemon->ubus;
> +  int ret;
> +
> +  if (!ubus || !notify)
>      return;
>  
> -  blob_buf_init(&b, 0);
> +  blob_buf_init(&b, BLOBMSG_TYPE_TABLE);
>    if (mac)
>      blobmsg_add_string(&b, "mac", mac);
>    if (ip)
> @@ -100,7 +170,9 @@ void ubus_event_bcast(const char *type, const char *mac, 
> const char
> *ip, const c
>    if (interface)
>      blobmsg_add_string(&b, "interface", interface);
>    
> -  ubus_notify(ubus, &ubus_object, type, b.head, -1);
> +  ret = ubus_notify(ubus, &ubus_object, type, b.head, -1);
> +  if (!ret)
> +    my_syslog(LOG_ERR, _("Failed to send UBus event: %s"), 
> ubus_strerror(ret));
>  }
> 


_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to