Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support #268

richnetdesign · 2020-12-05T02:36:28Z

resolves #267

Treats sighup as signal to close and reopen log file
Candump only supports autogenerated log file name by default. For use with logrotation daemon it expects consistent filename. I updated -l option to accept optional filename.
Since interface is a required argument, getopt skips final arg. Made -l take optional filename. Without filename maintains existing behavior.
Since sighup can interupt the select system call, now checking select error code for interupted syscal and logfile enabled. If those conditions are met, the errorcode is non critical.

Also attempt to resolve #38 as I was updating signal handling anyways.

A similar PR #189 was created to support integrated file rotation based on size. It however was not accepted due to increase bloat in candump, and not following unix philosophy of doing one thing well, and chaining functionality. This PR attempts to address these concerns by implementing the signal handler SIGHUP required, to integrate with existing log rotation tools.

hartkopp · 2020-12-05T20:29:34Z

Thanks for the reference to PR #189 from @BugLight which instantly came into my mind too.
I'm still not really happy with bloating candump in such way, as candump should be no file-handling-monster with tons of features but a tool that filters, shows and stores CAN frames.
IMO if we implement this kind of multi-chunk file generation we should support either the size based triggers as suggested by @BugLight AND the SIGHUP approach from you.
The PR #189 has different commits we can probably squash to a single patch. And once we have a separated function to hand-over the log stream to a new file we might also trigger this hand-over by SIGHUP.
What needs to be discussed is how the filenames are generated. We might add a postfix number to the original timstamp based filename or we generate a new timestamp based filename for each file (simple / fits better to SIGHUP).
I definitely do not like the change of the -l option.
Finally we would wait for PR #264 as new base for further developments.

marckleinebudde

Please use the kernel coding style:
https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst

marckleinebudde · 2020-12-05T20:28:44Z

candump.c

 extern int optind, opterr, optopt;

 static volatile int running = 1;
+static volatile int flag_reopen_file = 0;
+static volatile unsigned long sighup_count = 0;


no need to initialize statics with 0

marckleinebudde · 2020-12-05T20:28:45Z

candump.c

+FILE *logfile = NULL;
+static char log_filename[MAXLOGNAMESZ];
+
+unsigned char silent = SILENT_INI;


marckleinebudde · 2020-12-05T20:29:28Z

candump.c

+void sighup(int signo)
+{
+	if(signo == SIGHUP)
+	{


please use kernel coding style
if (signo == SIGHUP) {

marckleinebudde · 2020-12-05T20:29:47Z

candump.c

@@ -214,6 +233,100 @@ int idx2dindex(int ifidx, int socket) {
 	return i;
 }

+int sprint_auto_filename_format(char * buffer)
+{


(char *buffer)

marckleinebudde · 2020-12-05T20:30:36Z

candump.c

+	return 0;
+}
+
+//opens file using global filehandle logfile


no c++ comments please

marckleinebudde · 2020-12-05T20:37:24Z

candump.c

+{
+	if(arg != 0)
+	{
+		size_t len = strnlen(arg,MAXLOGNAMESZ);


please add a newline after the variable declaration and add a space after the ,

marckleinebudde · 2020-12-05T20:37:32Z

candump.c

+		{
+			strncpy(log_filename,arg,MAXLOGNAMESZ-1);
+		}
+		else


marckleinebudde · 2020-12-05T20:38:09Z

candump.c

+		else
+		{
+			//print_usage(basename(argv[0]));
+			exit(1);


please return the error and handle it

marckleinebudde · 2020-12-05T20:38:18Z

candump.c

+		}
+		is_auto_log_name = 0;
+	}
+	else


marckleinebudde · 2020-12-05T20:38:47Z

candump.c

-	signal(SIGTERM, sigterm);
-	signal(SIGHUP, sigterm);
-	signal(SIGINT, sigterm);
+	{


why do you open a new scope here?

marckleinebudde

Please fix the remaining coding style and then squash all patches, that there is only one.

marckleinebudde · 2020-12-06T09:23:33Z

candump.c

+static volatile int flag_reopen_file;
+static volatile unsigned long sighup_count;
+
+static int is_auto_log_name = 0;


no need to initialize static vars with 0

There are general issues with this patch. See #268 (comment)
It does not make sense to discuss style problems until the general approach and concept is under discussion.

IMO if we implement this kind of multi-chunk file generation we should support either the size based triggers as suggested by @BugLight AND the SIGHUP approach from you.

What do you mean: "either...or" (as in one or the other) or "and" (as in both)?

Once file naming and new file generation is solved, triggering the switch to a new file due to SIGHUP or size limit is hopefully not that complicated.

Yes, this was my intention!
But the way the filename is generated still needs some discussion IMO.
And finally I thought the PR #189 to be a simpler starting point, from which we might add the SIGHUP handling. But that's probably just my view.

marckleinebudde · 2020-12-06T09:23:55Z

candump.c

@@ -259,14 +257,13 @@ int sprint_auto_filename_format(char * buffer)
 	return 0;
 }

-//opens file using global filehandle logfile
-int open_log_file(const char * fileName)
+/*opens file using global var logfile*/


please add a space before and after the *

/* comment */

marckleinebudde · 2020-12-06T09:25:35Z

candump.c

+		const size_t len = strnlen(arg, MAXLOGNAMESZ);
+
+		if (len > 0 && len < MAXLOGNAMESZ) {
+			strncpy(log_filename, arg, MAXLOGNAMESZ-1);


MAXLOGNAMESZ - 1

marckleinebudde · 2020-12-06T09:26:33Z

candump.c

-		sigaction(SIGHUP, &usr_action, NULL);
-	}
-
+	/*Configure SIGHUP handler to reopen file if logging*/


/* add space */

marckleinebudde · 2020-12-06T09:26:56Z

candump.c

+	/*Configure SIGHUP handler to reopen file if logging*/
+	sigset_t sig_block_mask;
+	sigfillset(&sig_block_mask);
+	struct sigaction usr_action = { .sa_mask = sig_block_mask, .sa_flags = SA_RESTART };


please move the variable declaration to the beginning of the scope

marckleinebudde · 2020-12-06T09:27:27Z

candump.c

-			if(reopen_file(logfile) != 0)
-			{
+		if (flag_reopen_file == 1) { 
+			if(reopen_file(logfile) != 0) {


BugLight · 2020-12-06T14:20:06Z

@hartkopp, for me, it seems that name files using only timestamp (no part number) is more general and better approach.

richnetdesign · 2020-12-06T21:00:30Z

Thanks for the feedback everybody. I have rebased on the recent commits to master, and most importantly the epoll changes.

Regarding file name generation I want the ability to choose the output filename/format.
Of course I also want to support auto formatted names for convenience and backwards compatibility. The path of least resistance approach I initially took was making -l take an optional file name. I would be open to discussing other options, such as a separate parameter for log file name / format.

Below is a simplified example of a logrotate setup that I am looking to support. When triggered, logrotate copies that static filename to configured name / path, then sends SIGHUP. The idea is that previous data is archived, and candump recreates the original filename and continues logging.

/etc/logrotate.d/candump1

/tmp/candump1.log {
    hourly
    missingok
    compress
    maxsize 1K
    su root root
    rotate 50
    dateext
    dateformat -%Y%m%d_%H.%M.%s
    postrotate
        /usr/bin/kill -HUP `cat /var/run/candump1.pid 2> /dev/null`
    endscript
}

./candump -l /tmp/candump1.log vcan0 & echo $! > /var/run/candump1.pid

sudo logrotate /etc/logrotate.conf

ls /tmp/candump1*

hartkopp · 2020-12-06T21:42:27Z

When triggered, logrotate copies that static filename to configured name / path, then sends SIGHUP. The idea is that previous data is archived, and candump recreates the original filename and continues logging.

This sounds strange! You COPY the file and THEN issue the SIGHUP? Isn't there the chance that you record some frames twice?

This code makes a very clear close/open sequence between two CAN frames:
21f4b81#diff-c9165277a67241a5db54c2140829d4ebda95c35ce668d8ea27832afb24bf2dc5R779

./candump -l /tmp/candump1.log vcan0 & echo $! > /var/run/candump1.pid

Instead of your suggestion candump -l filename [other options] you can also run candump -L [other options] > filename :-)

richnetdesign · 2020-12-06T21:51:57Z

Sorry I meant move/rename the file. This is the debug output of related logrotated action.

renaming /tmp/candump1.log to /tmp/candump1.log-20201206_16.51.1607291480
creating new /tmp/candump1.log mode = 0664 uid = 1000 gid = 1000
running postrotate script
running script with args /tmp/candump1.log /tmp/candump1.log-20201206_16.51.1607291480: "
        /usr/bin/kill -HUP `cat /var/run/candump1.pid 2> /dev/null`
"

richnetdesign · 2020-12-07T07:36:39Z

you can also run candump -L [other options] > filename :-)

It is possible to do something like this, but with >> always append mode and logrotated, but it is less ideal, as it is possible to lose data. This is because it requires the "copytruncate" option, which copies the file, and truncates it so the original filehandle is preserved.
In contrast the example I provided integrates with candump to move then close the file handle, and makes a new replacement file handle pointing to the original log location. This ensures a clean hand off from one file to another.

I agree that logrotate is kind of weird. However, it is widely deployed so building in compatibility would be helpful. I like the idea of unifying the internal rotation functionality to internally use SIGHUP to trigger the size based rotation events.

This code makes a very clear close/open sequence between two CAN frames:
21f4b81#diff-c9165277a67241a5db54c2140829d4ebda95c35ce668d8ea27832afb24bf2dc5R779

Since a SIGHUP can occur at any time (even when can messages aren't being received), I am currently setting a flag on the signal and handling the rotation in the main loop. I also needed to handle the error result from interrupting epoll with sighup signal, so i also rotate/reopen file immediately if this occurs.

1. Treats sighup as signal to close and reopen log file 2. Candump only supports autogenerated log file name by default. For use with logrotation daemon it expects consistent filename. I updated -l option to accept optional filename. 3. Since interface is a required argument, getopt skips final arg. Made -l take optional filename. Without filename maintains existing behavior. 4. Since sighup can interupt the select system call, now checking epoll error code for interupted syscal and logfile enabled. If those conditions are met, the errorcode is non critical.

hartkopp · 2020-12-07T12:26:57Z

candump.c

+
+			switch (optopt)
+			{
+			case 'l':
+				log = 1;
+
+				if (process_logname_arg(optarg) != 0) {
+					print_usage(basename(argv[0]));
+					exit(1);
+				}
+				break;
+			default:
+				fprintf(stderr, "option -%c is missing a required argument\n", optopt);
+				return EXIT_FAILURE;
+			}		
+			break;

 		case 'l':
 			log = 1;
+
+			if (process_logname_arg(optarg) != 0) {
+				is_auto_log_name = 0;
+				print_usage(basename(argv[0]));
+				exit(1);
+			}


Can you please think about an integration into a logrotated configuration, that can cope with the assumptions:

The option '-l' remains untouched

The logfile is closed and a new logfile is opened with a new date-generated filename

So no re-open/append stuff and no change with the naming.
You might also execute candump in /tmp/candump/ to place the files there for logrotation post-processing.

I'm fine with creating two file splitting triggers about the length and the SIGHUP. But the current approach just adds too much complexity that does not need to be inside candump.

ilanbiala · 2022-02-22T16:42:59Z

Is there any movement on this PR or the linked one? It would be nice to have log rotation when recording very long logs for lengthy tests.

jayceslesar · 2022-05-22T15:17:57Z

+1 on getting this in

hartkopp · 2022-05-22T18:45:26Z

The infrastructure to create individual filenames changed in ad250a6 and c70d0a8 which solved my main concern to not break the -l command line option.

So the original patch needs to be reworked and rebased.

Additionally it has to be discussed whether the logfile names are autogenerated with date/time or should be based on an individual filename with an increasing counter (see #268 (comment)).

Edit:
The changes after epoll_wait also need to be updated.

Suggestion, which would could add the signal handling and file splitting without adding new options:

Use of option -f <fname> in a way that the last character gets a special functionality.

-f fname -> stores the logfile content into the file fname

-f name_$ -> stores the content into name_<unixtimestamp>.log e.g. name_1644943535.log which would make it easy to sort/preprocess the resulting files.

richnetdesign changed the title ~~Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support~~ #267 Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support Dec 5, 2020

richnetdesign changed the title ~~#267 Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support~~ Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support Dec 5, 2020

marckleinebudde requested changes Dec 5, 2020

View reviewed changes

richnetdesign force-pushed the feature/267-candump-reopen-log-on-sighup branch 3 times, most recently from 92806ce to 60be78c Compare December 6, 2020 08:17

marckleinebudde requested changes Dec 6, 2020

View reviewed changes

hartkopp mentioned this pull request Dec 6, 2020

Improved support for log rotation with candump #267

Closed

richnetdesign force-pushed the feature/267-candump-reopen-log-on-sighup branch from 60be78c to 1730bb4 Compare December 6, 2020 20:28

richnetdesign force-pushed the feature/267-candump-reopen-log-on-sighup branch from 1730bb4 to b3d0b3c Compare December 6, 2020 21:07

richnetdesign force-pushed the feature/267-candump-reopen-log-on-sighup branch from b3d0b3c to fd7ad60 Compare December 7, 2020 07:57

richnetdesign force-pushed the feature/267-candump-reopen-log-on-sighup branch from fd7ad60 to 3b5cbc0 Compare December 7, 2020 10:13

hartkopp reviewed Dec 7, 2020

View reviewed changes

hartkopp mentioned this pull request Oct 3, 2022

candump: Prevent file I/O blocking socket reading #381

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support #268

Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support #268

richnetdesign commented Dec 5, 2020 •

edited

hartkopp commented Dec 5, 2020

marckleinebudde left a comment

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde Dec 5, 2020

marckleinebudde left a comment

marckleinebudde Dec 6, 2020

hartkopp Dec 6, 2020 •

edited

marckleinebudde Dec 6, 2020

hartkopp Dec 6, 2020

marckleinebudde Dec 6, 2020

marckleinebudde Dec 6, 2020

marckleinebudde Dec 6, 2020

marckleinebudde Dec 6, 2020

marckleinebudde Dec 6, 2020

BugLight commented Dec 6, 2020

richnetdesign commented Dec 6, 2020 •

edited

hartkopp commented Dec 6, 2020

richnetdesign commented Dec 6, 2020

richnetdesign commented Dec 7, 2020 •

edited

hartkopp Dec 7, 2020 •

edited

ilanbiala commented Feb 22, 2022

jayceslesar commented May 22, 2022

hartkopp commented May 22, 2022 •

edited

Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support #268

Are you sure you want to change the base?

Update signal handlers to use sigaction, and re-open logfile on SIGHUP to integrate logrotated support #268

Conversation

richnetdesign commented Dec 5, 2020 • edited

hartkopp commented Dec 5, 2020

marckleinebudde 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

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

Choose a reason for hiding this comment

marckleinebudde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartkopp Dec 6, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugLight commented Dec 6, 2020

richnetdesign commented Dec 6, 2020 • edited

hartkopp commented Dec 6, 2020

richnetdesign commented Dec 6, 2020

richnetdesign commented Dec 7, 2020 • edited

hartkopp Dec 7, 2020 • edited

Choose a reason for hiding this comment

ilanbiala commented Feb 22, 2022

jayceslesar commented May 22, 2022

hartkopp commented May 22, 2022 • edited

richnetdesign commented Dec 5, 2020 •

edited

hartkopp Dec 6, 2020 •

edited

richnetdesign commented Dec 6, 2020 •

edited

richnetdesign commented Dec 7, 2020 •

edited

hartkopp Dec 7, 2020 •

edited

hartkopp commented May 22, 2022 •

edited