Re: [PATCH v4 19/27] wrapper.c: wrapper to open a file, fprintf then close

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 4, 2014, 02:47 UTC
Message-ID: <CAPig+cQT33-h2nmRDD8FC7KrCH-1UPqK39Vb3qHKSKz8nhtndQ@mail.gmail.com>
In-Reply-To: <531214BC.3070704@web.de>

On Sat, Mar 1, 2014 at 12:11 PM, Torsten Bögershausen <tboegi@web.de> wrote:

Show 35 quoted lines

> On 2014-03-01 13.12, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  cache.h   |  2 ++
>>  wrapper.c | 31 +++++++++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index 98b5dd3..99b86d9 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1239,6 +1239,8 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
>>  {
>>       return write_in_full(fd, str, strlen(str));
>>  }
>> +__attribute__((format (printf,3,4)))
>> +extern int write_file(const char *path, int fatal, const char *fmt, ...);
>>
>>  /* pager.c */
>>  extern void setup_pager(void);
>> diff --git a/wrapper.c b/wrapper.c
>> index 0cc5636..5ced50d 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -455,3 +455,34 @@ struct passwd *xgetpwuid_self(void)
>>                   errno ? strerror(errno) : _("no such user"));
>>       return pw;
>>  }
>> +
>> +int write_file(const char *path, int fatal, const char *fmt, ...)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
>> +     va_list params;
>> +     if (fd < 0) {

Micro nit atop Torsten's micro nit:

It is 3% easier to understand the code if the check for open() failure immediately follows the open() attempt:

    va_list params;
    int fd = open(...);
    if (fd < 0) {

Show 17 quoted lines

>> +             if (fatal)
>> +                     die_errno(_("could not open %s for writing"), path);
>> +             return -1;
>> +     }
>> +     va_start(params, fmt);
>> +     strbuf_vaddf(&sb, fmt, params);
>> +     va_end(params);
>> +     if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
>> +             int err = errno;
>> +             close(fd);
>> +             errno = err;
>> +             strbuf_release(&sb);
> Micro nit:
> Today we now what strbuf_release() is doing, but if we ever change the
> implementation, it is 3% safer to keep err a little bit longer like this:
>> +             strbuf_release(&sb);
>> +             errno = err;