| Message ID | 6a5c3e8e-0216-8b63-38fa-b7b19331d752@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] xopen: explicitly report creation failures | expand |
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? > + 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? Either way "Reviewed-by", and indeed a nice cleanup. Carlo
Am 26.08.21 um 01:46 schrieb Carlo Arenas: > 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. > >> + 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é
René Scharfe <l.s.r@web.de> writes: > Am 26.08.21 um 01:46 schrieb Carlo Arenas: >> 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. 100% agreed. >>> + 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. I fully agree that such a change, if done, must be done in an unrelated patch. It is funny that the code is already prepared for such a case where RDWR is defined as RDONLY|WRONLY. I wonder if we wrote the series of comparisons in this order on purpose, or we were just lucky, when we did 3ff53df7 (wrapper: implement xopen(), 2015-08-04) ;-) > >> Either way "Reviewed-by", and indeed a nice cleanup. > > Thank you! Yes, indeed, this is nicely done.
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); + 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);
If the flags O_CREAT and O_EXCL are both given then open(2) is supposed to create the file and error out if it already exists. The error message in that case looks like this: fatal: could not open 'foo' for writing: File exists Without further context this is confusing: Why should the existence of the file pose a problem? Isn't that a requirement for writing to it? Add a more specific error message for that case to tell the user that we actually don't expect the file to preexist, so the example becomes: fatal: unable to create 'foo': File exists Signed-off-by: René Scharfe <l.s.r@web.de> --- wrapper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.33.0