Re: [PATCH 1/2] xopen: explicitly report creation failures

From: René Scharfe <l.s.r@web.de>
Date: Aug 26, 2021, 15:23 UTC
Message-ID: <f44bf316-efae-34ce-33e0-0161c3bb78a0@web.de>
In-Reply-To: <CAPUEspjkcV1_R5DNXCkL5wQpZCW+K4As2nGuEGu6fyeFrr15KQ@mail.gmail.com>

Am 26.08.21 um 01:46 schrieb Carlo Arenas:

Show 15 quoted lines

> On Wed, Aug 25, 2021 at 2:11 PM René Scharfe <l.s.r@web.de> wrote:
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index 563ad590df..7c6586af32 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
>>                 if (errno == EINTR)
>>                         continue;
>>
>> -               if ((oflag & O_RDWR) == O_RDWR)
>> +               if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
>> +                       die_errno(_("unable to create '%s'"), path);
>
> probably over conservative, but && errno == EEXIST?

No matter what error we got, if O_CREAT and O_EXCL were both given then we tried to create a file, so this message applies.

Show 10 quoted lines

>
>> +               else if ((oflag & O_RDWR) == O_RDWR)
>>                         die_errno(_("could not open '%s' for reading and writing"), path);
>>                 else if ((oflag & O_WRONLY) == O_WRONLY)
>>                         die_errno(_("could not open '%s' for writing"), path);
>
> Since you are already changing this code, why not take the opportunity
> to refactor it
> and remove the " == FLAG" part of these conditionals which is
> otherwise redundant?

The repetition is unsightly, but it's a different issue that should be addressed separately. Simply removing the comparison feels iffy, though. POSIX doesn't seem to forbid e.g. O_RDONLY to be 1, O_WRONLY to be 2 and O_RDWR to be 3, and then you need to check all masked bits. I can't think of simpler alternative to the comparison.

> Either way "Reviewed-by", and indeed a nice cleanup.

Thank you!

René