Hi Ron,
thanks for your effort in sharing your fixes.
I have troubles understanding your patch, please find some comments
inline below.
~ Jo
--
On 07/11/2017 09:04 PM, Ron Brash wrote:
> This patch fixes a logread starvation error, which occurs after many
> logs are generated (around 16k if defaults are used). The log read
Do you have an easy test case for this? Is piping ~16K plaintext to the
logger enough to trigger the starvation? I'd like to understand the
problem some more before judging the patch.
> process seems to halt silently and yet continues running. A restart
> of the log services fixes it.
>
> This is problematic because logs should be logged, instead of silently lost
>
> Signed-off-by: “Ron Brash <“ron.br...@gmail.com”>
> ---
> log/logread.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/log/logread.c b/log/logread.c
> index edac1d9..7eb028e 100644
> --- a/log/logread.c
> +++ b/log/logread.c
> @@ -230,8 +230,10 @@ static void logread_fd_data_cb(struct ustream *s,
> int bytes)
> break;
>
> cur_len = blob_len(a) + sizeof(*a);
> - if (len < cur_len)
> + if (len < cur_len) {
> + ustream_consume(s, len);
> break;
> + }
>
> log_notify(a);
> ustream_consume(s, cur_len);
> @@ -240,14 +242,28 @@ static void logread_fd_data_cb(struct ustream
> *s, int bytes)
> uloop_end();
> }
>
> +static void notify_fd_remove_cb();
If you move the static function here you do not need the forward
declaration.
> +
> static void logread_fd_cb(struct ubus_request *req, int fd)
> {
> static struct ustream_fd test_fd;
> -
> + uloop_register_notify_fd_remove(fd, notify_fd_remove_cb);
What is "uloop_register_notify_fd_remove()" ? It is not part of libubox
as far as I can see. Is it a function introduced by some other custom
changes of yours?
> test_fd.stream.notify_read = logread_fd_data_cb;
> ustream_fd_init(&test_fd, fd);
> }
>
> +struct ubus_context *context;
> +uint32_t ctx_id;
> +struct blob_buf * bb;
> +struct ubus_request request;
The "request" and "ctx_id" variables appear to be local to
"notify_fd_remove_cb()" and should be moved inside the function.
The "context" and "bb" variables should be declared "static" if they're
outside of the function context, however I am not sure if we actually
need them because those values can be usually obtained elsewhere in the
callback.
> +
> +static void notify_fd_remove_cb() {
> + ubus_lookup_id(context, "log", &ctx_id);
> + ubus_invoke_async(context, ctx_id, "read", bb->head, &request);
> + request.fd_cb = logread_fd_cb;
> + ubus_complete_request_async(context, &request);
> +}
> +
> int main(int argc, char **argv)
> {
> static struct ubus_request req;
> @@ -362,6 +378,9 @@ int main(int argc, char **argv)
> sender.fd = STDOUT_FILENO;
> }
>
> + context = ctx;
> + bb = &b;
> +
> ubus_invoke_async(ctx, id, "read", b.head, &req);
> req.fd_cb = logread_fd_cb;
> ubus_complete_request_async(ctx, &req);
> @@ -374,3 +393,4 @@ int main(int argc, char **argv)
>
> return ret;
> }
> +
> --
> 2.7.4
>
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
