Re: [ubus PATCH] libubus: remove global variables
Hi Simon,
On 1/5/23 15:30, Simon Tate wrote:
Remove the use of global blob_buf and blob_attr variables to allow
for better thread safety with a ctx per thread on client invoke
and sends.
Add the same variables to within each calling function's scope,
encapsulating the memory usage there.
Fixes a multithreaded use case and has been verified 10,000
threads multiple times running invokes and send events.
I like this approach. The disadvantage is that we have more mallocs in
the code now. Before the global b variable had a pointer to the heap
which was reused, now we always start with a small buffer on the heap
and then have to realloc to increase it and free it up when the function
terminates.
The OpenWrt applications do not make much use of pthreads, but if your
application is such a change is helpful. I think we can effort the extra
overhead of the extra mallocs and frees.
I also found there many style problems and some problems in error paths
in the code.
Signed-off-by: Simon Tate
---
cli.c | 38 --
libubus-internal.h | 2 +-
libubus-io.c | 10 ---
libubus-obj.c | 63 ++-
libubus-req.c | 44 +-
libubus-sub.c | 13 ++---
libubus.c | 67 ++
7 files changed, 161 insertions(+), 76 deletions(-)
diff --git a/cli.c b/cli.c
index 81591ec..e6d7a1b 100644
--- a/cli.c
+++ b/cli.c
@@ -16,7 +16,6 @@
#include
#include "libubus.h"
-static struct blob_buf b;
static int listen_timeout;
static int timeout = 30;
static bool simple_output = false;
@@ -140,18 +139,29 @@ static int ubus_cli_call(struct ubus_context *ctx, int
argc, char **argv)
if (argc < 2 || argc > 3)
return -2;
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
if (argc == 3 && !blobmsg_add_json_from_string(, argv[2])) {
if (!simple_output)
fprintf(stderr, "Failed to parse message data\n");
- return -1;
+ ret = -1;
+ goto error;
}
ret = ubus_lookup_id(ctx, argv[0], );
- if (ret)
- return ret;
+ if (ret) {
+ goto error;
+ }
Please do not use brackets for only one statement. This is part of the
Linux kernel code style which is used here. There are multiple places
where you do not have to add them.
+
+ ret = ubus_invoke(ctx, id, argv[1], b.head, receive_call_result_data,
NULL, timeout * 1000);
+ if (ret) {
+ goto error;
+ }
- return ubus_invoke(ctx, id, argv[1], b.head, receive_call_result_data, NULL, timeout * 1000);
+error:
+ blob_buf_free();
+
+ return ret;
}
struct cli_listen_data {
@@ -265,15 +275,23 @@ static int ubus_cli_send(struct ubus_context *ctx, int
argc, char **argv)
if (argc < 1 || argc > 2)
return -2;
+ int ret = UBUS_STATUS_OK;
Initialization is not needed here.
Please move the "int ret;" to the beginning of the function.
+
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
if (argc == 2 && !blobmsg_add_json_from_string(, argv[1])) {
if (!simple_output)
fprintf(stderr, "Failed to parse message data\n");
- return -1;
+ ret = -1;
+ goto error;
}
- return ubus_send_event(ctx, argv[0], b.head);
+ ret = ubus_send_event(ctx, argv[0], b.head);
+
+error:
+ blob_buf_free();
+ return ret;
}
struct cli_wait_data {
@@ -428,6 +446,7 @@ ubus_cli_get_monitor_data(struct blob_attr *data)
struct blob_attr *tb[UBUS_ATTR_MAX];
int i;
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
blob_parse(data, tb, policy, UBUS_ATTR_MAX);
@@ -454,7 +473,10 @@ ubus_cli_get_monitor_data(struct blob_attr *data)
}
}
- return blobmsg_format_json(b.head, true);
+ char* ret = blobmsg_format_json(b.head, true);
+
+ blob_buf_free();
+ return ret;
}
static void
diff --git a/libubus-internal.h b/libubus-internal.h
index 24477a0..5b23668 100644
--- a/libubus-internal.h
+++ b/libubus-internal.h
@@ -17,7 +17,7 @@
extern struct blob_buf b;
extern const struct ubus_method watch_method;
-struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len);
+void ubus_parse_msg(struct blob_attr *msg, size_t len, struct blob_attr
**attrbuf, size_t attrbuf_len);
bool ubus_validate_hdr(struct ubus_msghdr *hdr);
void ubus_handle_data(struct uloop_fd *u, unsigned int events);
int ubus_send_msg(struct ubus_context *ctx, uint32_t seq,
diff --git a/libubus-io.c b/libubus-io.c
index 3561ac4..143d3be 100644
--- a/libubus-io.c
+++ b/libubus-io.c
@@ -41,12 +41,10 @@ static const struct blob_attr_info
ubus_policy[UBUS_ATTR_MAX] = {
[ubus PATCH] libubus: remove global variables
Remove the use of global blob_buf and blob_attr variables to allow
for better thread safety with a ctx per thread on client invoke
and sends.
Add the same variables to within each calling function's scope,
encapsulating the memory usage there.
Fixes a multithreaded use case and has been verified 10,000
threads multiple times running invokes and send events.
Signed-off-by: Simon Tate
---
cli.c | 38 --
libubus-internal.h | 2 +-
libubus-io.c | 10 ---
libubus-obj.c | 63 ++-
libubus-req.c | 44 +-
libubus-sub.c | 13 ++---
libubus.c | 67 ++
7 files changed, 161 insertions(+), 76 deletions(-)
diff --git a/cli.c b/cli.c
index 81591ec..e6d7a1b 100644
--- a/cli.c
+++ b/cli.c
@@ -16,7 +16,6 @@
#include
#include "libubus.h"
-static struct blob_buf b;
static int listen_timeout;
static int timeout = 30;
static bool simple_output = false;
@@ -140,18 +139,29 @@ static int ubus_cli_call(struct ubus_context *ctx, int
argc, char **argv)
if (argc < 2 || argc > 3)
return -2;
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
if (argc == 3 && !blobmsg_add_json_from_string(, argv[2])) {
if (!simple_output)
fprintf(stderr, "Failed to parse message data\n");
- return -1;
+ ret = -1;
+ goto error;
}
ret = ubus_lookup_id(ctx, argv[0], );
- if (ret)
- return ret;
+ if (ret) {
+ goto error;
+ }
+
+ ret = ubus_invoke(ctx, id, argv[1], b.head, receive_call_result_data,
NULL, timeout * 1000);
+ if (ret) {
+ goto error;
+ }
- return ubus_invoke(ctx, id, argv[1], b.head, receive_call_result_data,
NULL, timeout * 1000);
+error:
+ blob_buf_free();
+
+ return ret;
}
struct cli_listen_data {
@@ -265,15 +275,23 @@ static int ubus_cli_send(struct ubus_context *ctx, int
argc, char **argv)
if (argc < 1 || argc > 2)
return -2;
+ int ret = UBUS_STATUS_OK;
+
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
if (argc == 2 && !blobmsg_add_json_from_string(, argv[1])) {
if (!simple_output)
fprintf(stderr, "Failed to parse message data\n");
- return -1;
+ ret = -1;
+ goto error;
}
- return ubus_send_event(ctx, argv[0], b.head);
+ ret = ubus_send_event(ctx, argv[0], b.head);
+
+error:
+ blob_buf_free();
+ return ret;
}
struct cli_wait_data {
@@ -428,6 +446,7 @@ ubus_cli_get_monitor_data(struct blob_attr *data)
struct blob_attr *tb[UBUS_ATTR_MAX];
int i;
+ struct blob_buf b = { 0 };
blob_buf_init(, 0);
blob_parse(data, tb, policy, UBUS_ATTR_MAX);
@@ -454,7 +473,10 @@ ubus_cli_get_monitor_data(struct blob_attr *data)
}
}
- return blobmsg_format_json(b.head, true);
+ char* ret = blobmsg_format_json(b.head, true);
+
+ blob_buf_free();
+ return ret;
}
static void
diff --git a/libubus-internal.h b/libubus-internal.h
index 24477a0..5b23668 100644
--- a/libubus-internal.h
+++ b/libubus-internal.h
@@ -17,7 +17,7 @@
extern struct blob_buf b;
extern const struct ubus_method watch_method;
-struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len);
+void ubus_parse_msg(struct blob_attr *msg, size_t len, struct blob_attr
**attrbuf, size_t attrbuf_len);
bool ubus_validate_hdr(struct ubus_msghdr *hdr);
void ubus_handle_data(struct uloop_fd *u, unsigned int events);
int ubus_send_msg(struct ubus_context *ctx, uint32_t seq,
diff --git a/libubus-io.c b/libubus-io.c
index 3561ac4..143d3be 100644
--- a/libubus-io.c
+++ b/libubus-io.c
@@ -41,12 +41,10 @@ static const struct blob_attr_info
ubus_policy[UBUS_ATTR_MAX] = {
[UBUS_ATTR_SUBSCRIBERS] = { .type = BLOB_ATTR_NESTED },
};
-static struct blob_attr *attrbuf[UBUS_ATTR_MAX];
-__hidden struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len)
+__hidden void ubus_parse_msg(struct blob_attr *msg, size_t len, struct
blob_attr **attrbuf, size_t attrbuf_len)
{
- blob_parse_untrusted(msg, len, attrbuf, ubus_policy, UBUS_ATTR_MAX);
- return attrbuf;
+ blob_parse_untrusted(msg, len, attrbuf, ubus_policy, attrbuf_len);
}
static void wait_data(int fd, bool write)
@@ -137,6 +135,8 @@ int __hidden ubus_send_msg(struct ubus_context *ctx,
uint32_t seq,
hdr.seq = cpu_to_be16(seq);
hdr.peer = cpu_to_be32(peer);
+ struct blob_buf b = {0};
+
if (!msg) {
blob_buf_init(, 0);
msg = b.head;
@@ -152,6 +152,8 @@ int __hidden ubus_send_msg(struct ubus_context *ctx,
uint32_t seq,
if (fd >= 0)
