Re: [Dnsmasq-discuss] [PATCH] Improve UBus support
On Fri, 2019-03-29 at 21:52 +, 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
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
-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 = _object_type,
.methods =
Re: [Dnsmasq-discuss] [PATCH] Improve UBus support
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
>
> -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
[Dnsmasq-discuss] [PATCH] Improve UBus support
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
-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 = _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" :
