Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Florian Weimer <fw at deneb dot enyo dot de>]

* Paul Eggert:

> On 1/22/20 12:03 PM, Florian Weimer wrote:
>> +	  if (errno == ENFILE || errno == EMFILE)
>> +	    /* These errors cannot happen with a straight fchmodat
>> +	       operation because it does not create file descriptors,
>> +	       so hide them.  */
>> +	    __set_errno (EOPNOTSUPP);
>> +	  /* Otherwise, this should accurately reflect the expected
>> +	     error from fchmodat (e.g., EBADF or ENOENT).  */
>
> These lines can be omitted. I omitted them when recently adding similar code to 
> Gnulib in <https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fchmodat.c>. 
> There's no need for fchmodat to masquerade ENFILE and EMFILE as EOPNOTSUPP: 
> POSIX does not require it and masquerading those two errno values would be 
> misleading, in the sense that it would make it harder for the caller to diagnose 
> what actually went wrong.

Hmm.  The error code is really obscure, particularly with AT_FDCWD,
where there is no file descriptor involved.  I hope that we can
eventually make it go away with proper kernel support.  But I see your
point about telling transient errors (such as ENOMEM) from more
persistent ones.

>> +      char buf[32];
>> +      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
>> +	{
>> +	  __close_nocancel (pathfd);
>> +	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);
>> +	}
>
> Similarly here. Also, there should be no reason for snprintf to fail. One 
> possibility is to replace this with what's in Gnulib:
>
>    static char const fmt[] = "/proc/self/fd/%d";
>    char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
>    sprintf (buf, fmt, pathfd);

I think we should add a proper wrapper for this eventually, where the
buffer allocation in the caller is properly abstracted via a
suitable struct type.

> where INT_BUFSIZE_BOUND is taken from <intprops.h>.
>
> Otherwise, this patch series looks OK to me; thanks for writing it.

Thanks.

What do you think about introducing new symbol versions vs keeping
things as they are in the patch today?

If we do not introduce new symbol versions, we should strive to
backport this change widely, so that users do not run into obscure
failures.