Re: [libvirt] [PATCH] util: Implement and use virFileIsRegular() rather than d_type

On 06/06/2018 11:37 AM, Stefan Berger wrote: > The dirent's d_type field is not portable to all platforms. So we have > to use stat() to determine the type of file for the functions that need > to be cross-platform. Fix virFileChownFiles() by calling the new > virFileIsRegular() function. > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- >   src/libvirt_private.syms |  1 + >   src/util/virfile.c       | 17 +++++++++++++---- >   src/util/virfile.h       |  1 + >   3 files changed, 15 insertions(+), 4 deletions(-) > + > +bool > +virFileIsRegular(const char *path) > +{ > +    struct stat s; > +    return (stat(path, &s) == 0) && S_ISREG(s.st_mode); This always does a stat. > +} > + > + >   /** >    * virFileExists: Check for presence of file >    * @path: Path of file to check > @@ -3005,12 +3014,13 @@ int virFileChownFiles(const char *name, >           return -1; >         while ((direrr = virDirRead(dir, &ent, name)) > 0) { > -        if (ent->d_type != DT_REG) > -            continue; And this was the non-portable code you're getting rid of, which was already buggy - even on systems with d_type, a return of DT_UNKNOWN requires a stat() rather than a blind continue. However, your replacement is pessimizing platforms where d_type is reliable, by making us do a stat() where we previously did not need to. Better would be using macros that use d_type where possible, and fall back to stat() only when d_type does not exist or is DT_UNKNOWN; for example, here's how gnulib does it: #if HAVE_STRUCT_DIRENT_D_TYPE /* True if the type of the directory entry D is known.  */ # define DT_IS_KNOWN(d) ((d)->d_type != DT_UNKNOWN) /* True if the type of the directory entry D must be T.  */ # define DT_MUST_BE(d, t) ((d)->d_type == (t)) # define D_TYPE(d) ((d)->d_type) #else # define DT_IS_KNOWN(d) false # define DT_MUST_BE(d, t) false # define D_TYPE(d) DT_UNKNOWN # undef DT_UNKNOWN # define DT_UNKNOWN 0 (although we have to be careful with licensing, because that is from lib/fts.c which is marked GPLv3+).