logrotate: update the logdir path whenever it changes #501
Conversation
lxbsz
force-pushed
the
logrotate_logdir
branch
from
83b39e9
to
a10f8a6
Compare
|
@mikechristie @pkalever |
| } | ||
|
|
||
| p += m; | ||
| } |
There was a problem hiding this comment.
did you miss to free(line) in the loop ?
There was a problem hiding this comment.
No needed to free it every time:
If *lineptr is NULL, then getline() will allocate a buffer for storing the line, which should be freed by the user program. (In this case, the value in *n is
ignored.)
Alternatively, before calling getline(), *lineptr can contain a pointer to a malloc(3)-allocated buffer *n bytes in size. If the buffer is not large enough to
hold the line, getline() resizes it with realloc(3), updating *lineptr and *n as necessary.
There was a problem hiding this comment.
Yeah, I read the manpage now. Thanks for adding the clarity.
libtcmu_log.c
Outdated
| return -errno; | ||
| } | ||
|
|
||
| ret = fseek(fp,0L,SEEK_END); |
There was a problem hiding this comment.
coding style: space after comma
| @@ -524,6 +525,75 @@ static bool tcmu_log_dir_check(const char *path) | |||
| return true; | |||
| } | |||
|
|
|||
| #define TCMU_LOGROTATE_PATH "/etc/logrotate.d/tcmu-runner" | |||
There was a problem hiding this comment.
Should this be part of a header file ?
There was a problem hiding this comment.
Currently, only used here. IMO, both will be okay.
There was a problem hiding this comment.
Oh yes, this is perfectly okay.
My intention was, considering declaration of potentially common macros (such as this) in headers will reduce rework.
libtcmu_log.c
Outdated
| fp = fopen(TCMU_LOGROTATE_PATH, "r+"); | ||
| if (fp == NULL) { | ||
| tcmu_err("Failed to open file '%s', %m\n", TCMU_LOGROTATE_PATH); | ||
| return -errno; |
There was a problem hiding this comment.
You need to save errno before line 539 and then return it.
| ret = fseek(fp,0L,SEEK_END); | ||
| if (ret == -1) { | ||
| tcmu_err("Failed to seek file '%s', %m\n", TCMU_LOGROTATE_PATH); | ||
| ret = -errno; |
There was a problem hiding this comment.
this should be moved before line 545
| len = ftell(fp); | ||
| if (len == -1) { | ||
| tcmu_err("Failed to get the length of file '%s', %m\n", TCMU_LOGROTATE_PATH); | ||
| ret = -errno; |
lxbsz
force-pushed
the
logrotate_logdir
branch
from
a10f8a6
to
363d0aa
Compare
| @@ -524,6 +525,75 @@ static bool tcmu_log_dir_check(const char *path) | |||
| return true; | |||
| } | |||
|
|
|||
| #define TCMU_LOGROTATE_PATH "/etc/logrotate.d/tcmu-runner" | |||
There was a problem hiding this comment.
Oh yes, this is perfectly okay.
My intention was, considering declaration of potentially common macros (such as this) in headers will reduce rework.
| } | ||
|
|
||
| p += m; | ||
| } |
There was a problem hiding this comment.
Yeah, I read the manpage now. Thanks for adding the clarity.
Signed-off-by: Xiubo Li <[email protected]>
lxbsz
force-pushed
the
logrotate_logdir
branch
from
363d0aa
to
5e6b5e0
Compare
| @@ -536,7 +609,8 @@ static int tcmu_log_dir_set(const char *log_dir) | |||
|
|
|||
| tcmu_log_dir_free(); | |||
| tcmu_log_dir = new_dir; | |||
| return 0; | |||
|
|
|||
| return tcmu_logrotate_conf_set(tcmu_log_dir); | |||
There was a problem hiding this comment.
I think you need to put this call before we free the old and set the new tcmu_log_dir in case the call to tcmu_logrotate_conf_set fails. We then do not have to worry about unraveling what was done here.
Fixes: #500
Signed-off-by: Xiubo Li [email protected]