| Message ID | 1463564732-9315-1-git-send-email-dnbugnar@ocedo.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 18/05/16 10:45, Dan Bugnar wrote: > + newest = (_log + (newest - log)); > newest->size = 0; > - oldest = log = _log; > + memset(newest, 0, size - log_size); > + oldest = (_log + (oldest - log)); > + log = _log; > log_end = ((void*) log) + size; Reallocating a circular buffer is non-trival. I'm concerned it doesn't cover all combinations of: (larger size OR smaller size) AND (newest > oldest OR oldest > newest) Also is there a need for locking during the log_buffer_reinit()? Conor
On 18/05/16 15:03, Dan Bugnar wrote: > For the larger size it takes the distance from the old log and newest > and added to _log to have the new newest, same for the oldest. It > doesn't matter if the newest > oldest OR oldest > newest. If the oldest > newest, there will be a blank area, the new space. How will this be handled during log printing? > And if the size is smaller than we will have a fresh new smaller > buffer with the newest and oldest pointing to _log(start). > > If it is possible that a message is added to the old log after the > realloc call is done and the log doesn't point to the new buffer, we > will loose that message. For both of these situations, it is possible to lose messages. I thought the intention was not to lose messages? Conor
On 18/05/2016 16:08, Conor O'Gorman wrote: > On 18/05/16 15:03, Dan Bugnar wrote: >> For the larger size it takes the distance from the old log and newest >> and added to _log to have the new newest, same for the oldest. It >> doesn't matter if the newest > oldest OR oldest > newest. > If the oldest > newest, there will be a blank area, the new space. How > will this be handled during log printing? > >> And if the size is smaller than we will have a fresh new smaller >> buffer with the newest and oldest pointing to _log(start). >> >> If it is possible that a message is added to the old log after the >> realloc call is done and the log doesn't point to the new buffer, we >> will loose that message. > For both of these situations, it is possible to lose messages. I thought > the intention was not to lose messages? > > Conor > i see it the same way. please check the code and make sure it covers all cases. 2/2 looks good but i just marked the series as "Changes requested" please explain in V3 how you addressed the issue of reallocating the buffer. John > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
diff --git a/log/syslog.c b/log/syslog.c index e8b6774..d6cb868 100644 --- a/log/syslog.c +++ b/log/syslog.c @@ -42,7 +42,7 @@ #define PAD(x) (x % 4) ? (((x) - (x % 4)) + 4) : (x) static char *log_dev = LOG_DEFAULT_SOCKET; -static int log_size = LOG_DEFAULT_SIZE; +static int log_size = 0; static struct log_head *log, *log_end, *oldest, *newest; static int current_id = 0; static regex_t pat_prio; @@ -237,34 +237,30 @@ log_list(int count, struct log_head *h) } int -log_buffer_init(int size) +log_buffer_reinit(int size) { - struct log_head *_log = malloc(size); + if (size <= 0) + size = LOG_DEFAULT_SIZE; + if (log_size == size) + return 0; + + struct log_head *_log = realloc(log, size); if (!_log) { + oldest = newest = log = NULL; fprintf(stderr, "Failed to initialize log buffer with size %d\n", log_size); return -1; } - memset(_log, 0, size); - if (log && ((log_size + sizeof(struct log_head)) < size)) { - struct log_head *start = _log; - struct log_head *end = ((void*) _log) + size; - struct log_head *l; - - l = log_list(0, NULL); - while ((start < end) && l && l->size) { - memcpy(start, l, PAD(sizeof(struct log_head) + l->size)); - start = (struct log_head *) &l->data[PAD(l->size)]; - l = log_list(0, l); - } - free(log); - newest = start; + newest = (_log + (newest - log)); newest->size = 0; - oldest = log = _log; + memset(newest, 0, size - log_size); + oldest = (_log + (oldest - log)); + log = _log; log_end = ((void*) log) + size; } else { + memset(_log, 0, size); oldest = newest = log = _log; log_end = ((void*) log) + size; } @@ -276,13 +272,10 @@ log_buffer_init(int size) void log_init(int _log_size) { - if (_log_size > 0) - log_size = _log_size; - regcomp(&pat_prio, "^<([0-9]*)>(.*)", REG_EXTENDED); regcomp(&pat_tstamp, "^\[[ 0]*([0-9]*).([0-9]*)] (.*)", REG_EXTENDED); - if (log_buffer_init(log_size)) { + if (log_buffer_reinit(_log_size)) { fprintf(stderr, "Failed to allocate log memory\n"); exit(-1); } diff --git a/log/syslog.h b/log/syslog.h index 81a039f..ed5a41b 100644 --- a/log/syslog.h +++ b/log/syslog.h @@ -35,7 +35,7 @@ void log_shutdown(void); typedef void (*log_list_cb)(struct log_head *h); struct log_head* log_list(int count, struct log_head *h); -int log_buffer_init(int size); +int log_buffer_reinit(int size); void log_add(char *buf, int size, int source); void ubus_notify_log(struct log_head *l);