Re: [PATCH v2 1/4] cli: add global option --stderr=FILE

[Tomi Ollila <tomi.ollila@iki.fi>]

On Tue, May 28 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Tomi Ollila on May 26 at 11:45 am:
>> With this option all writes to stderr are redirected to the spesified
>> FILE (or to stdout on case FILE is '-'). This is immediately useful
>> in emacs interface as some of its exec intefaces do not provide
>> separation of stdout and stderr.
>> ---
>>  notmuch-client.h |  1 +
>>  notmuch.c        | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>> 
>> diff --git a/notmuch-client.h b/notmuch-client.h
>> index 45749a6..4a3c7ac 100644
>> --- a/notmuch-client.h
>> +++ b/notmuch-client.h
>> @@ -54,6 +54,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t;
>>  #include <sys/stat.h>
>>  #include <sys/time.h>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include <dirent.h>
>>  #include <errno.h>
>>  #include <signal.h>
>> diff --git a/notmuch.c b/notmuch.c
>> index f51a84f..654a568 100644
>> --- a/notmuch.c
>> +++ b/notmuch.c
>> @@ -251,6 +251,32 @@ notmuch_command (notmuch_config_t *config,
>>      return 0;
>>  }
>>  
>> +static int
>> +redirect_stderr (const char * stderr_file)
>> +{
>> +    if (strcmp (stderr_file, "-") == 0) {
>> +	if (dup2 (STDOUT_FILENO, STDERR_FILENO) < 0) {
>> +	    perror ("dup2");
>> +	    return 1;
>> +	}
>> +    } else {
>> +	int fd = open (stderr_file, O_WRONLY|O_CREAT|O_APPEND, 0644);
>
> I think this should include O_TRUNC; otherwise it's too error-prone to
> use programmatically. 

You're right! I thought this one night and was supposed check that it is
O_TRUNC there -- forgot and obviously it wasn't ;/

> The permissions should be 0666 (if the user's
> umask says things should be group or world writable, it's not our job
> to disagree).

I agree that 0644 is incorrect -- but IMHO it should be 0600 -- when the
data is written to /tmp world-readable file gave others chance to read
potentially sensitive information from that file...

I'll ask this (also) in IRC -- if people generally agree the bits should
be 0666 I'll do that change instead (even personally opposing ATM).

I'll also do the man and NEWS chances suggested for v3.

Thanks for the review (this also goes to Jani & Mark).

Tomi

>> +	if (fd < 0) {
>> +	    fprintf (stderr, "Error: Cannot redirect stderr to '%s': %s\n",
>> +		     stderr_file, strerror (errno));
>> +	    return 1;
>> +	}
>> +	if (fd != STDERR_FILENO) {
>> +	    if (dup2 (fd, STDERR_FILENO) < 0) {
>> +		perror ("dup2");
>> +		return 1;
>> +	    }
>> +	    close (fd);
>> +	}
>> +    }
>> +    return 0;
>> +}
>> +
>>  int
>>  main (int argc, char *argv[])
>>  {
>> @@ -259,6 +285,7 @@ main (int argc, char *argv[])
>>      const char *command_name = NULL;
>>      command_t *command;
>>      char *config_file_name = NULL;
>> +    char *stderr_file = NULL;
>>      notmuch_config_t *config;
>>      notmuch_bool_t print_help=FALSE, print_version=FALSE;
>>      int opt_index;
>> @@ -268,6 +295,7 @@ main (int argc, char *argv[])
>>  	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
>>  	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
>>  	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
>> +	{ NOTMUCH_OPT_STRING, &stderr_file, "stderr", '\0', 0 },
>>  	{ 0, 0, 0, 0, 0 }
>>      };
>>  
>> @@ -287,6 +315,10 @@ main (int argc, char *argv[])
>>  	return 1;
>>      }
>>  
>> +    if (stderr_file && redirect_stderr (stderr_file) != 0) {
>> +	/* error already printed */
>> +	return 1;
>> +    }
>>      if (print_help)
>>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>>  
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch