log: Add option to re-open a log file #326

chrissie-c · 2018-10-01T08:57:56Z

With an optional new filename

chrissie-c · 2018-10-01T10:03:31Z

This changes the API of the custom loggers (to take a struct timespec rather than a time_t) but we're already changing the soname of the library anyway for v2.0.0

wferi · 2018-10-01T10:22:20Z

lib/log_file.c

 	}
-	t->instance = fopen(t->filename, "a+");
+	newfile = fopen(t->filename, "a+");


Maybe insert a comment here that the caller can do the error handling because errno is preserved? And hope qb_log_target_user_data_set() always respects this constraint... Maybe add a comment there as well? Ugh... Rather, what about changing _file_reload() (a little used function) to return the error code explicitly for better transparency (letting the code document itself)?

On a higher level, is it a good idea to close() before attempting to open()? What do you prefer to stay with (or without) on failure? For example, strange things can happen if logrotate compresses a file still being appended to, but that may still be better than a total loss of logs. On the other hand, if the IPC used by the application for triggering log rotation facilitates reporting errors, logrotate can be informed about the failure. (Synchronous IPC also avoids the race between close() and compression without resorting to delaycompress, which is not a proper solution in technical sense anyway.) I think it might be better to leave this choice to the application: if the reopen fails, it should be able to continue logging to the old location knowing it reported the error to the requester of the reopen, or just close the logs and optionally prepare for some kind of recovery.

Good points all. I've pushed a new commit to fix them

wferi · 2018-10-01T11:10:35Z

This changes the API of the custom loggers (to take a struct timespec rather than a time_t)

How does time representation influence log reopening? Thanks.

chrissie-c · 2018-10-01T11:18:49Z

This changes the API of the custom loggers (to take a struct timespec rather than a time_t)

How does time representation influence log reopening? Thanks.

It doesn't - this is a different PR I'm still working on, sorry. Blame it on Mondays!

wferi · 2018-10-01T14:05:28Z

lib/log_file.c


-	if (!target->instance) {
+	if (rc) {
 		return -errno;
 	} else {
 		return 0;


Why not move this part into _do_file_reload() so that you can simply return _do_file_reload(t) here? Gets rid of a conditional and moves reading of errno much closer to the function setting it, which adds to clarity IMO.

We can't do that because _do_file_reload() is called from the logger object and that doesn't return an error (bad design, I know, not guilty). That's why I had to split it into two functions in the first place!

You mean _file_reload(), don't you? You could still define it to errno = -_do_file_reload(t, NULL) I guess (using the second argument for passing in the new filename as discussed in my other comment). Then get rid of it altogether with the next API change.

wferi · 2018-10-01T14:12:05Z

lib/log_file.c

@@ -132,13 +148,14 @@ int32_t
 qb_log_file_reopen(int32_t t, const char *filename)
 {
 	struct qb_log_target *target = qb_log_target_get(t);
+	int rc;

 	if (filename) {
 		(void)strlcpy(target->filename, filename, PATH_MAX);


Isn't changing the filename field premature here? If open() fails, we keep the old FILE structure after all. I overlooked this added complication; looks like one must pass the new file name to _do_file_reload() in a separate argument...

Possibly. But t->filename is only used to open the file anyway.

I believe you. It might be useful to close this avenue of future confusion anyway.

chrissie-c · 2018-10-02T08:06:46Z

I've changed the reopen to only close the old file if the new one succeeds, and moved the fclose to be sure that errno is retained on return.

Return code from _file_reload() I'm going to leave as-is for the moment. Changing the logger api is a bigger job and should be kept al together IMO.

wferi

I think it's a fine addition to the API. Thanks!

wferi · 2018-10-08T13:31:48Z

lib/log_file.c

 	}
 }

 static int
-_do_file_reload(int32_t target)
+_do_file_reload(const char *filename, int32_t target)
 {
 	struct qb_log_target *t = qb_log_target_get(target);


Just wondering: why qb_log_target_get() does not check the validity of target?

wferi · 2018-10-08T14:00:16Z

lib/log_file.c

-		(void)strlcpy(target->filename, filename, PATH_MAX);
-	}
-	rc = _do_file_reload(t);
+	rc = _do_file_reload(filename, t);


Why not return -errno; /* from fopen() */ instead of return -1; in _do_file_reload() so that you can then rename it to int32_t qb_log_file_reopen() and happily be one function down?

wferi · 2018-10-08T14:09:07Z

lib/log_file.c

-		fclose(t->instance);
+	/* Only close oldfile if newfile succeeds */
+	if (oldfile && newfile) {
+		fclose(oldfile);


I'd write this as if (oldfile) { fclose(oldfile); } inside the if (newfile) { } block below for clarity. YMMV.

chrissie-c · 2018-10-09T14:22:29Z

Chatting with honza it seems we have opened a large can of worms here, the whole reconfig locking is going to need an overhaul.

wferi · 2018-10-10T08:51:55Z

Wouldn't dropping support for changing the log file name during reopen help a little? That's somewhat unrelated anyway and remains possible by stopping and starting logging the old way.

jfriesse · 2018-10-10T10:19:26Z

@wferi Not too much. Current logging thread is simply not protected from changing configuration at all (so qb_log_ctl2) and errors are visible even when something as simple as reload corosync.conf is performed (it basically works just by accident).

and add API call to reopen a log file.

chrissie-c · 2018-10-12T12:48:55Z

To clarify what's happening here. I force pushed a new patch yesterday that adds locking around all of the logging reconfiguration. Today's small amendment is to make 'in_logger' atomic as it's accessed across threads and helgrind was complaining (it still complains but only because it only understands pthreads).

jfriesse · 2018-10-15T09:30:13Z

Last update seems to solve all problems with corosync-cfgtool -R and logging , so ACK by me.

Looks like some of @wferi's comments are still unresolved (Why not return -errno; ...) so probably not yet ready to go.

It's tidier

wferi · 2018-10-15T11:23:38Z

Yes, thanks, this last change makes fairly explicit where I was going to with that idea: _do_file_reload() is just qb_log_file_reopen() with the arguments swapped, so in my opinion it would be better to not introduce it at all, but call qb_log_file_reopen() from _file_reload().

chrissie-c · 2018-10-15T11:30:44Z

Thanks. I'd rather leave it as it is TBH, then the two 'API' functions are equal and the fn that does the work is called by both.

wferi · 2018-10-15T17:47:46Z

Your call, certainly. I probably wouldn't have brought it up if _do_file_reload() wasn't introduced in this very pull request. If you prefer to have this symmetry between _file_reload() and qb_log_file_reopen(), then be it.

chrissie-c · 2018-10-16T07:10:02Z

Thanks.

wferi reviewed Oct 1, 2018

View reviewed changes

jfriesse mentioned this pull request Oct 8, 2018

log: Implement support for reopening log files corosync/corosync#388

Closed

wferi approved these changes Oct 8, 2018

View reviewed changes

chrissie-c added 2 commits October 11, 2018 14:19

log: Add locking round config changes

fccc83e

and add API call to reopen a log file.

log: Make in_logger into an atomic as it's used across threads.

603352c

log: Return -errno from _do_file_reload

3bd1fb8

It's tidier

chrissie-c merged commit 6032d21 into ClusterLabs:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log: Add option to re-open a log file #326

log: Add option to re-open a log file #326

chrissie-c commented Oct 1, 2018

chrissie-c commented Oct 1, 2018

wferi Oct 1, 2018

chrissie-c Oct 1, 2018

wferi commented Oct 1, 2018

chrissie-c commented Oct 1, 2018

wferi Oct 1, 2018

chrissie-c Oct 1, 2018

wferi Oct 1, 2018

wferi Oct 1, 2018

chrissie-c Oct 1, 2018

wferi Oct 1, 2018

chrissie-c commented Oct 2, 2018

wferi left a comment

wferi Oct 8, 2018

wferi Oct 8, 2018

wferi Oct 8, 2018

chrissie-c commented Oct 9, 2018

wferi commented Oct 10, 2018

jfriesse commented Oct 10, 2018

chrissie-c commented Oct 12, 2018

jfriesse commented Oct 15, 2018

wferi commented Oct 15, 2018

chrissie-c commented Oct 15, 2018

wferi commented Oct 15, 2018

chrissie-c commented Oct 16, 2018

log: Add option to re-open a log file #326

log: Add option to re-open a log file #326

Conversation

chrissie-c commented Oct 1, 2018

chrissie-c commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wferi commented Oct 1, 2018

chrissie-c commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrissie-c commented Oct 2, 2018

wferi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrissie-c commented Oct 9, 2018

wferi commented Oct 10, 2018

jfriesse commented Oct 10, 2018

chrissie-c commented Oct 12, 2018

jfriesse commented Oct 15, 2018

wferi commented Oct 15, 2018

chrissie-c commented Oct 15, 2018

wferi commented Oct 15, 2018

chrissie-c commented Oct 16, 2018