Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()

> Hi Paul,
>
> On 2015-06-18 13:25, Paul Tan wrote:
>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 0cc7ae8..bc77d77 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -719,6 +719,7 @@ extern void *xrealloc(void *ptr, size_t size);
>>  extern void *xcalloc(size_t nmemb, size_t size);
>>  extern void *xmmap(void *start, size_t length, int prot, int flags,
>> int fd, off_t offset);
>>  extern void *xmmap_gently(void *start, size_t length, int prot, int
>> flags, int fd, off_t offset);
>> +extern int xopen(const char *path, int flags, ...);
>
> I wonder whether it is worth it to make this a varargs function. It is not too much to ask callers to specify a specific mode everytime they call `xopen()`, no?
>
>> diff --git a/wrapper.c b/wrapper.c
>> index c1a663f..82658b3 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)
>>  # endif
>>  #endif
>>
>> +/**
>> + * xopen() is the same as open(), but it die()s if the open() fails.
>> + */
>> +int xopen(const char *path, int oflag, ...)
>> +{
>> +     mode_t mode = 0;
>> +     va_list ap;
>> +
>> +     va_start(ap, oflag);
>> +     if (oflag & O_CREAT)
>> +             mode = va_arg(ap, mode_t);
>> +     va_end(ap);
>> +
>> +     assert(path);
>> +
>> +     for (;;) {
>> +             int fd = open(path, oflag, mode);
>> +             if (fd >= 0)
>> +                     return fd;
>> +             if (errno == EINTR)
>> +                     continue;
>> +             die_errno(_("could not open '%s'"), path);
>
> It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like
>
> if (oflag & O_WRITE)
>     die_errno(_("could not open '%s' for writing"), path);
> else if (oflag & O_READ)
>     die_errno(_("could not open '%s' for reading"), path);
> else
>     die_errno(_("could not open '%s'"), path);
>
> ? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.

>
> Ciao,
> Dscho
>
>