Re: [PATCH 1/1] linux: open and openat ignore 'mode' with O_TMPFILE in flags

[Eric Rannaud <e at nanocritical dot com>]

On Thu, Oct 30, 2014 at 3:39 PM, Roland McGrath <roland@hack.frob.com> wrote:
> > Introduce a system-specific macro __OPEN_NEEDS_MODE(oflag): by default,
> > it tests for O_CREAT in oflag; on Linux, it tests for either O_CREAT or
> > __O_TMPFILE (even if __USE_GNU is not defined).
>
> This doesn't really need to be system-specific.  The values of O_*
> constants are system-specific, and even what subset of all known O_*
> constants are made available is system-specific, but the meaning of each
> given O_FOO constant in the API (including things like whether it implies
> using a mode argument for open/openat) is part of the generic glibc API.
>
> So certainly a convenience macro is worthwhile to avoid repeating a
> nontrivial expression.  But it doesn't need to be system-specific.
> It can just be based on #if.

Understood. In io/fcntl.h:

  #ifdef __O_TMPFILE
  # define __OPEN_NEEDS_MODE(oflag) \
    ( ((oflag) & O_CREAT) != 0 || ((oflag) & __O_TMPFILE) == __O_TMPFILE )
  #else
  # define __OPEN_NEEDS_MODE(oflag) ( ((oflag) & O_CREAT) != 0 )
  #endif

Do I test for ifdef __O_TMPFILE or for O_TMPFILE (only available with
__USE_GNU)? Is the glibc itself compiled with __USE_GNU?


> >  __errordecl (__open_missing_mode,
> > -          "open with O_CREAT in second argument needs 3 arguments");
> > +          "open with e.g. O_CREAT in second argument needs 3 arguments");
>
> Keep these explicit about the flags required.  It's OK to say "O_CREAT or
> O_TMPFILE" even if O_TMPFILE might not actually exist.  For extra credit,
> you could make the checks more specific and different errors for the
> O_CREAT and O_TMPFILE cases.  But there is not much benefit to that.

OK.

> > +      if (__OPEN_NEEDS_MODE(__oflag) && __va_arg_pack_len () < 1)
>
> Space before paren.  I won't cite all the other places with the same issue.

Thanks.

> >  /* Open FILE and return a new file descriptor for it, or -1 on error.
> > -   OFLAG determines the type of access used.  If O_CREAT is on OFLAG,
> > +   OFLAG determines the type of access used.  If __OPEN_NEEDS_MODE(OFLAG),
> >     the third argument is taken as a `mode_t', the mode of the created file.
>
> Don't make comment changes like this.  The comments in the public header
> file are there to be user documentation of the API.  So they should never
> mention internal symbols or whatnot, only the aspects of the API that
> someone writing a call to this function would use directly.  Again, it's
> fine to say "... or O_TMPFILE" without further qualification regardless of
> whether O_TMPFILE is actually available.

Thanks, I misunderstood the target audience for these comments.

Eric

-- 
Eric Rannaud <e@nanocritical.com>
Nanocritical, CEO