Re: [PATCH 12/14] lib: provide mkostemp fallback function

[Ruediger Meier <sweet_f_a@gmx.de>]

On Friday 26 February 2016, Yuriy M. Kaminskiy wrote:
> Ruediger Meier <sweet_f_a@gmx.de> writes:
> > From: Ruediger Meier <ruediger.meier@ga-group.nl>
> >
> > It's missing on OSX.
> >
> > Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
> > ---
> >  configure.ac        |  1 +
> >  include/fileutils.h |  4 ++++
> >  lib/fileutils.c     | 27 +++++++++++++++++++++++++++
> >  libblkid/src/save.c |  1 +
> >  4 files changed, 33 insertions(+)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 727a875..64a16ff 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -368,6 +368,7 @@ AC_CHECK_FUNCS([ \
> >  	llseek \
> >  	lseek64 \
> >  	mempcpy \
> > +	mkostemp \
> >  	nanosleep \
> >  	ntp_gettime \
> >  	personality \
> > diff --git a/include/fileutils.h b/include/fileutils.h
> > index ba8da7f..7c5594e 100644
> > --- a/include/fileutils.h
> > +++ b/include/fileutils.h
> > @@ -8,6 +8,10 @@
> >
> >  #include "c.h"
> >
> > +#ifndef HAVE_MKOSTEMP
> > +extern int mkostemp(char *template, int flags);
> > +#endif
> > +
> >  extern int xmkstemp(char **tmpname, const char *dir, const char
> > *prefix);
> >
> >  static inline FILE *xfmkstemp(char **tmpname, const char *dir,
> > const char *prefix) diff --git a/lib/fileutils.c b/lib/fileutils.c
> > index bf8e60a..05a8c02 100644
> > --- a/lib/fileutils.c
> > +++ b/lib/fileutils.c
> > @@ -13,6 +13,33 @@
> >  #include "fileutils.h"
> >  #include "pathnames.h"
> >
> > +#ifndef HAVE_MKOSTEMP
> > +int mkostemp(char *template, int flags)
> > +{
> > +	int fd, old_flags, errno_save;
> > +
> > +	fd = mkstemp(template);
> > +	if (fd < 0)
> > +		return fd;
> > +
> > +	old_flags = fcntl(fd, F_GETFD);
>
> This cannot be right. `flags` in `mkostemp` is **open** flags (such

Oops, this was really stupid.

> as `O_RDWR`, `O_CREAT`, etc; most notable, `O_CLOEXEC`; however, if
> `O_CLOEXEC` is missing on system, "c.h" defines it as 0, so it is
> silently ignored on those systems, instead of being emulated; so,
> whenever it matters, callers must call **both** `open(O_CLOEXEC)` and
> `fcntl(F_SETFD,FD_CLOEXEC)`]).

I see, but then defining O_CLOEXEC to 0 at all is the big mistake. We 
are using (ignoring) it in whole util-linux. To fix that we have to add 
fcntl(F_SETFD,FD_CLOEXEC) everywhere.

> fcntl(F_SETFD) sets only FD_CLOEXEC (bit 0), which certainly
> different.
>
> (Note that `F_SETFL` is not proper replacement either, as many `O_*`
> flags make sense in open(), but not in `fcntl(F_SETFL)` [e.g. O_CREAT
> or O_EXCL]; and it is *not* correct to `or` old flags and new ones;
> e.g. if old flags contains `O_RDWR`, and new flags is O_RDONLY,
> `old_flags | flags` may be nonsense).

Ok, to have it most simple I would now only implement a fallback for 
exactly this usage:
  mkostemp(localtmp, O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC);

like this

#if defined HAVE_MKOSTEMP && defined O_CLOEXEC
mkostemp(localtmp, O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC);
#else
fd = mkstemp(template);
old_flags = fcntl(fd, F_GETFD);
fcntl(fd, F_SETFD, old_flags | FD_CLOEXEC)
#endif


> > +	if (old_flags < 0)
> > +		goto unwind;
> > +	if (fcntl(fd, F_SETFD, old_flags | flags) < 0)
> > +		goto unwind;
> > +
> > +	return fd;
> > +
> > +unwind:
> > +	errno_save = errno;
> > +	unlink(template);
> > +	close(fd);
> > +	errno = errno_save;
> > +
> > +	return -1;
> > +}
> > +#endif
> > +
> >  /* Create open temporary file in safe way.  Please notice that the
> >   * file permissions are -rw------- by default. */
> >  int xmkstemp(char **tmpname, const char *dir, const char *prefix)
> > diff --git a/libblkid/src/save.c b/libblkid/src/save.c
> > index 5e8bbee..b9f447a 100644
> > --- a/libblkid/src/save.c
> > +++ b/libblkid/src/save.c
> > @@ -23,6 +23,7 @@
> >  #endif
> >
> >  #include "closestream.h"
> > +#include "fileutils.h"
> >
> >  #include "blkidP.h"
>
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html