Re: [LEDE-DEV] [PATCH ubox v2] logread: Add support for output template

[Henry Chang]

Hi Philip,

Thanks for reviewing.

I re-submitted it here:
http://lists.infradead.org/pipermail/lede-dev/2017-May/007241.html

The only thing I didn't do is fmemopen() because I didn't see any
examples in the project.

Regards,

Henry Jr.

On Mon, May 1, 2017 at 10:48 AM, Philip Prindeville
 wrote:
> Inline…
>
>
>> On Apr 27, 2017, at 5:33 PM, Henry Chang  wrote:
>>
>> From: Henry Chang 
>>
>> Signed-off-by: Henry Chang 
>> ---
>> log/logread.c | 153 
>> --
>> 1 file changed, 127 insertions(+), 26 deletions(-)
>>
>> diff --git a/log/logread.c b/log/logread.c
>> index edac1d9..3e3406a 100644
>> --- a/log/logread.c
>> +++ b/log/logread.c
>> @@ -56,10 +56,25 @@ static const struct blobmsg_policy log_policy[] = {
>>   [LOG_TIME] = { .name = "time", .type = BLOBMSG_TYPE_INT64 },
>> };
>>
>> +enum {
>> + TPL_FIELD_MESSAGE,
>> + TPL_FIELD_PRIORITY,
>> + TPL_FIELD_SOURCE,
>> + TPL_FIELD_TIMESTAMP,
>> +};
>> +
>> +static const char *TPL_FIELDS[] = {
>> + [TPL_FIELD_MESSAGE] = "%message%",
>> + [TPL_FIELD_PRIORITY] = "%priority%",
>> + [TPL_FIELD_SOURCE] = "%source%",
>> + [TPL_FIELD_TIMESTAMP] = "%timestamp%",
>> +};
>> +
>> static struct uloop_timeout retry;
>> static struct uloop_fd sender;
>> static regex_t regexp_preg;
>> static const char *log_file, *log_ip, *log_port, *log_prefix, *pid_file, 
>> *hostname, *regexp_pattern;
>> +static const char *log_template;
>> static int log_type = LOG_STDOUT;
>> static int log_size, log_udp, log_follow, log_trailer_null = 0;
>> static int log_timestamp;
>> @@ -102,6 +117,7 @@ static int log_notify(struct blob_attr *msg)
>>   struct stat s;
>>   char buf[512];
>>   char buf_ts[32];
>> + char buf_p[32];
>>   uint32_t p;
>>   char *str;
>>   time_t t;
>> @@ -137,34 +153,108 @@ static int log_notify(struct blob_attr *msg)
>>   regexec(_preg, m, 0, NULL, 0) == REG_NOMATCH)
>>   return 0;
>>   t = blobmsg_get_u64(tb[LOG_TIME]) / 1000;
>> - if (log_timestamp) {
>> - t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
>> - snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ",
>> - (unsigned long)t, t_ms);
>> - }
>> + t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
>> + snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ", (unsigned long) t, 
>> t_ms);
>>   c = ctime();
>>   p = blobmsg_get_u32(tb[LOG_PRIO]);
>>   c[strlen(c) - 1] = '\0';
>>   str = blobmsg_format_json(msg, true);
>> + snprintf(buf_p, sizeof buf_p, "%u", p);
>
> Even on an 64BIT architecture, an unsigned will be 32-bits and therefore 10 
> decimal digits plus NUL…  not sure why buf_p[] needs to be so large.
>
>
>> +
>> + if (log_template) {
>> + char buf2[512];
>> + size_t len;
>> + int tpli = -1;
>> +
>> + if ((len = strlen(log_template) + 1) > sizeof buf) {
>> + fprintf(stderr, "template is larger than the internal 
>> buffer\n");
>> + return 1;
>> + }
>> + strncpy(buf, log_template, len);
>> +
>> + char *substr = buf;
>> + for (;;) {
>> + char *substrnext = NULL;
>> + for (int i = 0; i < sizeof TPL_FIELDS / sizeof (char 
>> *); ++i) {
>
>
> If ‘i’ is always a positive value, why not make it unsigned?
>
>
>> + char *substrbuf = strstr(substr, 
>> TPL_FIELDS[i]);
>> + if (!substrbuf)
>> + continue;
>> + if (!substrnext || substrbuf < substrnext) {
>> + substrnext = substrbuf;
>> + tpli = i;
>> + }
>> + }
>> + substr = substrnext;
>> + if (!substr)
>> + break;
>> + char *field = NULL;
>> + switch (tpli) {
>
>
> The rest of this file matches switch/case indent…  please go with that.
>
>
>> + case TPL_FIELD_MESSAGE:
>> + field = m;
>> + break;
>> + case TPL_FIELD_PRIORITY:
>> + field = buf_p;
>> + break;
>> + case TPL_FIELD_SOURCE:
>> + switch 
>> (blobmsg_get_u32(tb[LOG_SOURCE])) {
>> + case SOURCE_KLOG:
>> + field = "kernel";
>> + break;
>> +  

Re: [LEDE-DEV] [PATCH ubox v2] logread: Add support for output template

[Philip Prindeville]

Inline…


> On Apr 27, 2017, at 5:33 PM, Henry Chang  wrote:
> 
> From: Henry Chang 
> 
> Signed-off-by: Henry Chang 
> ---
> log/logread.c | 153 --
> 1 file changed, 127 insertions(+), 26 deletions(-)
> 
> diff --git a/log/logread.c b/log/logread.c
> index edac1d9..3e3406a 100644
> --- a/log/logread.c
> +++ b/log/logread.c
> @@ -56,10 +56,25 @@ static const struct blobmsg_policy log_policy[] = {
>   [LOG_TIME] = { .name = "time", .type = BLOBMSG_TYPE_INT64 },
> };
> 
> +enum {
> + TPL_FIELD_MESSAGE,
> + TPL_FIELD_PRIORITY,
> + TPL_FIELD_SOURCE,
> + TPL_FIELD_TIMESTAMP,
> +};
> +
> +static const char *TPL_FIELDS[] = {
> + [TPL_FIELD_MESSAGE] = "%message%",
> + [TPL_FIELD_PRIORITY] = "%priority%",
> + [TPL_FIELD_SOURCE] = "%source%",
> + [TPL_FIELD_TIMESTAMP] = "%timestamp%",
> +};
> +
> static struct uloop_timeout retry;
> static struct uloop_fd sender;
> static regex_t regexp_preg;
> static const char *log_file, *log_ip, *log_port, *log_prefix, *pid_file, 
> *hostname, *regexp_pattern;
> +static const char *log_template;
> static int log_type = LOG_STDOUT;
> static int log_size, log_udp, log_follow, log_trailer_null = 0;
> static int log_timestamp;
> @@ -102,6 +117,7 @@ static int log_notify(struct blob_attr *msg)
>   struct stat s;
>   char buf[512];
>   char buf_ts[32];
> + char buf_p[32];
>   uint32_t p;
>   char *str;
>   time_t t;
> @@ -137,34 +153,108 @@ static int log_notify(struct blob_attr *msg)
>   regexec(_preg, m, 0, NULL, 0) == REG_NOMATCH)
>   return 0;
>   t = blobmsg_get_u64(tb[LOG_TIME]) / 1000;
> - if (log_timestamp) {
> - t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
> - snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ",
> - (unsigned long)t, t_ms);
> - }
> + t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
> + snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ", (unsigned long) t, 
> t_ms);
>   c = ctime();
>   p = blobmsg_get_u32(tb[LOG_PRIO]);
>   c[strlen(c) - 1] = '\0';
>   str = blobmsg_format_json(msg, true);
> + snprintf(buf_p, sizeof buf_p, "%u", p);

Even on an 64BIT architecture, an unsigned will be 32-bits and therefore 10 
decimal digits plus NUL…  not sure why buf_p[] needs to be so large.


> +
> + if (log_template) {
> + char buf2[512];
> + size_t len;
> + int tpli = -1;
> +
> + if ((len = strlen(log_template) + 1) > sizeof buf) {
> + fprintf(stderr, "template is larger than the internal 
> buffer\n");
> + return 1;
> + }
> + strncpy(buf, log_template, len);
> +
> + char *substr = buf;
> + for (;;) {
> + char *substrnext = NULL;
> + for (int i = 0; i < sizeof TPL_FIELDS / sizeof (char 
> *); ++i) {


If ‘i’ is always a positive value, why not make it unsigned?


> + char *substrbuf = strstr(substr, TPL_FIELDS[i]);
> + if (!substrbuf)
> + continue;
> + if (!substrnext || substrbuf < substrnext) {
> + substrnext = substrbuf;
> + tpli = i;
> + }
> + }
> + substr = substrnext;
> + if (!substr)
> + break;
> + char *field = NULL;
> + switch (tpli) {


The rest of this file matches switch/case indent…  please go with that.


> + case TPL_FIELD_MESSAGE:
> + field = m;
> + break;
> + case TPL_FIELD_PRIORITY:
> + field = buf_p;
> + break;
> + case TPL_FIELD_SOURCE:
> + switch 
> (blobmsg_get_u32(tb[LOG_SOURCE])) {
> + case SOURCE_KLOG:
> + field = "kernel";
> + break;
> + case SOURCE_SYSLOG:
> + field = "syslog";
> + break;
> + case SOURCE_INTERNAL:
> + field = "internal";
> + break;
> + default:
> +   

[LEDE-DEV] [PATCH ubox v2] logread: Add support for output template

[Henry Chang]

From: Henry Chang 

Signed-off-by: Henry Chang 
---
 log/logread.c | 153 --
 1 file changed, 127 insertions(+), 26 deletions(-)

diff --git a/log/logread.c b/log/logread.c
index edac1d9..3e3406a 100644
--- a/log/logread.c
+++ b/log/logread.c
@@ -56,10 +56,25 @@ static const struct blobmsg_policy log_policy[] = {
[LOG_TIME] = { .name = "time", .type = BLOBMSG_TYPE_INT64 },
 };
 
+enum {
+   TPL_FIELD_MESSAGE,
+   TPL_FIELD_PRIORITY,
+   TPL_FIELD_SOURCE,
+   TPL_FIELD_TIMESTAMP,
+};
+
+static const char *TPL_FIELDS[] = {
+   [TPL_FIELD_MESSAGE] = "%message%",
+   [TPL_FIELD_PRIORITY] = "%priority%",
+   [TPL_FIELD_SOURCE] = "%source%",
+   [TPL_FIELD_TIMESTAMP] = "%timestamp%",
+};
+
 static struct uloop_timeout retry;
 static struct uloop_fd sender;
 static regex_t regexp_preg;
 static const char *log_file, *log_ip, *log_port, *log_prefix, *pid_file, 
*hostname, *regexp_pattern;
+static const char *log_template;
 static int log_type = LOG_STDOUT;
 static int log_size, log_udp, log_follow, log_trailer_null = 0;
 static int log_timestamp;
@@ -102,6 +117,7 @@ static int log_notify(struct blob_attr *msg)
struct stat s;
char buf[512];
char buf_ts[32];
+   char buf_p[32];
uint32_t p;
char *str;
time_t t;
@@ -137,34 +153,108 @@ static int log_notify(struct blob_attr *msg)
regexec(_preg, m, 0, NULL, 0) == REG_NOMATCH)
return 0;
t = blobmsg_get_u64(tb[LOG_TIME]) / 1000;
-   if (log_timestamp) {
-   t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
-   snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ",
-   (unsigned long)t, t_ms);
-   }
+   t_ms = blobmsg_get_u64(tb[LOG_TIME]) % 1000;
+   snprintf(buf_ts, sizeof(buf_ts), "[%lu.%03u] ", (unsigned long) t, 
t_ms);
c = ctime();
p = blobmsg_get_u32(tb[LOG_PRIO]);
c[strlen(c) - 1] = '\0';
str = blobmsg_format_json(msg, true);
+   snprintf(buf_p, sizeof buf_p, "%u", p);
+
+   if (log_template) {
+   char buf2[512];
+   size_t len;
+   int tpli = -1;
+
+   if ((len = strlen(log_template) + 1) > sizeof buf) {
+   fprintf(stderr, "template is larger than the internal 
buffer\n");
+   return 1;
+   }
+   strncpy(buf, log_template, len);
+
+   char *substr = buf;
+   for (;;) {
+   char *substrnext = NULL;
+   for (int i = 0; i < sizeof TPL_FIELDS / sizeof (char 
*); ++i) {
+   char *substrbuf = strstr(substr, TPL_FIELDS[i]);
+   if (!substrbuf)
+   continue;
+   if (!substrnext || substrbuf < substrnext) {
+   substrnext = substrbuf;
+   tpli = i;
+   }
+   }
+   substr = substrnext;
+   if (!substr)
+   break;
+   char *field = NULL;
+   switch (tpli) {
+   case TPL_FIELD_MESSAGE:
+   field = m;
+   break;
+   case TPL_FIELD_PRIORITY:
+   field = buf_p;
+   break;
+   case TPL_FIELD_SOURCE:
+   switch 
(blobmsg_get_u32(tb[LOG_SOURCE])) {
+   case SOURCE_KLOG:
+   field = "kernel";
+   break;
+   case SOURCE_SYSLOG:
+   field = "syslog";
+   break;
+   case SOURCE_INTERNAL:
+   field = "internal";
+   break;
+   default:
+   field = "-";
+   }
+   case TPL_FIELD_TIMESTAMP:
+   field = buf_ts;
+   break;
+   }
+   if (!field || tpli < 0)
+   continue;
+   *buf2 = '\0';
+   size_t fieldlen = strlen(field);