On Fri, 2019-03-29 at 21:52 +0000, Simon Kelley wrote:
> 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.

Good point. I've reworked my patch to only log errors once and attached it to 
this mail.
Thanks for reviewing it!

Regards,

  Jan Willem

From ee38d5bf46a5947a978d9209aee5142a2ce452c6 Mon Sep 17 00:00:00 2001
From: Jan Willem Janssen <j.w.jans...@lxtreme.nl>
Date: Mon, 25 Mar 2019 12:42:23 +0100
Subject: [PATCH] Improved UBus supported
To: dnsmasq-discuss@lists.thekelleys.org.uk

- aligned the handling of UBus connections with the DBus code as it
makes it a bit easier to comprehend;
- added logging to the various UBus calls to aid debugging from an
enduser point of view, but be careful to not flood the logs;
- show the (lack of) support for UBus in the configuration string.
---
 src/config.h  |   4 ++
 src/dnsmasq.c |  32 +++++++++++-
 src/dnsmasq.h |   6 +++
 src/ubus.c    | 138 ++++++++++++++++++++++++++++++++++++++++++--------
 4 files changed, 157 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..db5ef68 100644
--- a/src/dnsmasq.c
+++ b/src/dnsmasq.c
@@ -420,6 +420,16 @@ 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
+    {
+      daemon->ubus = NULL;
+      ubus_init();
+    }
+#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 +822,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 +1020,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 +1155,15 @@ 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)
+            {
+              ubus_init();
+            }
+
+          check_ubus_listeners();
+        }
 #endif
 
       check_dns_listeners(now);
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 0e409b4..11a443c 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
+void 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..c2cca22 100644
--- a/src/ubus.c
+++ b/src/ubus.c
@@ -20,29 +20,112 @@
 
 #include <libubus.h>
 
-static struct ubus_context *ubus = NULL;
 static struct blob_buf b;
+static int notify;
+static int error_logged = 0;
 
 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;
+}
+
+static 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, _("Cannot reconnect to UBus: %s"), ubus_strerror(ret));
+
+      ubus_destroy(ubus);
+    }
+}
+
+void ubus_init()
+{
+  struct ubus_context *ubus = NULL;
+  int ret = 0;
+
+  ubus = ubus_connect(NULL);
+  if (!ubus)
+    {
+      if (!error_logged)
+        {
+          my_syslog(LOG_ERR, _("Cannot initialize UBus: connection failed"));
+          error_logged = 1;
+        }
+
+      ubus_destroy(ubus);
+      return;
+    }
+
+  ret = ubus_add_object(ubus, &ubus_object);
+  if (ret)
+    {
+      if (!error_logged)
+        {
+          my_syslog(LOG_ERR, _("Cannot add object to UBus: %s"), ubus_strerror(ret));
+          error_logged = 1;
+        }
+      return;
+    }
+
+  ubus->connection_lost = ubus_disconnect_cb;
+  daemon->ubus = ubus;
+  error_logged = 0;
+
+  my_syslog(LOG_INFO, _("Connected to system UBus"));
+}
+
 void set_ubus_listeners()
 {
+  struct ubus_context *ubus = (struct ubus_context *)daemon->ubus;
   if (!ubus)
-    return;
+    {
+      if (!error_logged)
+        {
+          my_syslog(LOG_ERR, _("Cannot set UBus listeners: no connection"));
+          error_logged = 1;
+        }
+      return;
+    }
+
+  error_logged = 0;
 
   poll_listen(ubus->sock.fd, POLLIN);
   poll_listen(ubus->sock.fd, POLLERR);
@@ -51,46 +134,57 @@ 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);
+      if (!error_logged)
+        {
+          my_syslog(LOG_ERR, _("Cannot poll UBus listeners: no connection"));
+          error_logged = 1;
+        }
+      return;
     }
   
+  error_logged = 0;
+
   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 +194,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));
 }
 
 
-- 
2.20.1

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

Reply via email to