Patchwork logsave : Avoid unnecessary backgrounding of logsave in case of failures

login
register
mail settings
Submitter Manish Katiyar
Date Oct. 7, 2008, 5:44 p.m.
Message ID <ea11fea30810071044l6d8887e0udb52342f82412e4a@mail.gmail.com>
Download mbox | patch
Permalink /patch/3192/
State New
Headers show

Comments

Manish Katiyar - Oct. 7, 2008, 5:44 p.m.
Hi Ted,

I am not sure why we wan't to background the logsave and keep retrying
opening the fd in case of failures. But there may be situations when
we
will never be able to succeed and thus create unnecessary process. For
example invoking it

/home/mkatiyar/sbin> ./logsave /testfile ls

as a normal user will never succeed. Below patch adds some of the
error conditions where we can just avoid it.

Signed-off-by : Manish Katiyar <mkatiyar@gmail.com>

---
 misc/logsave.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)
Theodore Ts'o - Oct. 10, 2008, 6:31 p.m.
On Tue, Oct 07, 2008 at 11:14:11PM +0530, Manish Katiyar wrote:
> Hi Ted,
> 
> I am not sure why we wan't to background the logsave and keep retrying
> opening the fd in case of failures. 

That's one of the main reason why logsave exists; the filesystem
containing /var/log might not be mounted, or the root filesystem may
be mounted read-only, and so the log file can't be written until the
filesystem is remounted r/w or /var is mounted.

> But there may be situations when we will never be able to succeed
> and thus create unnecessary process. For example invoking it
> 
> /home/mkatiyar/sbin> ./logsave /testfile ls

The main use of logsave was in init.d scripts.  So I didn't really
worry about the permissoin denied case.  Perhaps logsave should just
fail hard and not even run the command if there is a permission denied
error.  That would certainly be simpler...

> +static void should_background(int err, int *nobackground) {
> +	switch (err) {
> +		case EPERM:
> +		case EACCES:
> +			*nobackground = err;
> +			break;
> +		default :
> +			*nobackground = 0;
> +	}
> +	return ;
> +}

Why is this its own function?

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Manish Katiyar - Oct. 10, 2008, 6:48 p.m.
On Sat, Oct 11, 2008 at 12:01 AM, Theodore Tso <tytso@mit.edu> wrote:
> On Tue, Oct 07, 2008 at 11:14:11PM +0530, Manish Katiyar wrote:
>> Hi Ted,
>>
>> I am not sure why we wan't to background the logsave and keep retrying
>> opening the fd in case of failures.
>
> That's one of the main reason why logsave exists; the filesystem
> containing /var/log might not be mounted, or the root filesystem may
> be mounted read-only, and so the log file can't be written until the
> filesystem is remounted r/w or /var is mounted.
>
>> But there may be situations when we will never be able to succeed
>> and thus create unnecessary process. For example invoking it
>>
>> /home/mkatiyar/sbin> ./logsave /testfile ls
>
> The main use of logsave was in init.d scripts.  So I didn't really
> worry about the permissoin denied case.  Perhaps logsave should just
> fail hard and not even run the command if there is a permission denied
> error.  That would certainly be simpler...
>
>> +static void should_background(int err, int *nobackground) {
>> +     switch (err) {
>> +             case EPERM:
>> +             case EACCES:
>> +                     *nobackground = err;
>> +                     break;
>> +             default :
>> +                     *nobackground = 0;
>> +     }
>> +     return ;
>> +}
>
> Why is this its own function?

I made it that way so that in future if we want to handle any more
error numbers to bail out , we just need to add a case statement.

Thanks -
Manish

>
>                                                - Ted
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/misc/logsave.c b/misc/logsave.c
index f0011f8..77a0a16 100644
--- a/misc/logsave.c
+++ b/misc/logsave.c
@@ -203,6 +203,17 @@  static int copy_from_stdin(void)
 	return 0;
 }

+static void should_background(int err, int *nobackground) {
+	switch (err) {
+		case EPERM:
+		case EACCES:
+			*nobackground = err;
+			break;
+		default :
+			*nobackground = 0;
+	}
+	return ;
+}


 int main(int argc, char **argv)
@@ -211,7 +222,7 @@  int main(int argc, char **argv)
 	char	*outfn, **cpp;
 	int	openflags = O_CREAT|O_WRONLY|O_TRUNC;
 	int	send_flag = SEND_LOG;
-	int	do_stdin;
+	int	do_stdin, nobackground = 0;
 	time_t	t;

 	while ((c = getopt(argc, argv, "+asv")) != EOF) {
@@ -237,6 +248,8 @@  int main(int argc, char **argv)
 	argc -= optind;

 	outfd = open(outfn, openflags, 0644);
+	if (outfd < 0)
+		should_background(errno, &nobackground);
 	do_stdin = !strcmp(argv[0], "-");

 	send_output("Log of ", 0, send_flag);
@@ -263,7 +276,7 @@  int main(int argc, char **argv)
 	send_output(ctime(&t), 0, send_flag);
 	send_output("----------------\n", 0, send_flag);

-	if (outbuf) {
+	if (!nobackground) {
 		pid = fork();
 		if (pid < 0) {
 			perror("fork");
@@ -282,6 +295,8 @@  int main(int argc, char **argv)
 		}
 		write(outfd, outbuf, outbufsize);
 		free(outbuf);
+	} else {
+		printf("Unable to save log to %s : %s\n", outfn, strerror(nobackground));
 	}
 	close(outfd);