[libvirt] [PATCH] util: Implement and use virFileIsRegular() rather than d_type
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@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 17 +++++++++++++---- src/util/virfile.h | 1 + 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43bbdef8f1..b4ab1f3629 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1796,6 +1796,7 @@ virFileIsDir; virFileIsExecutable; virFileIsLink; virFileIsMountPoint; +virFileIsRegular; virFileIsSharedFS; virFileIsSharedFSType; virFileLength; diff --git a/src/util/virfile.c b/src/util/virfile.c index e41881f6c9..12b41a64e0 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1851,6 +1851,15 @@ virFileIsDir(const char *path) return (stat(path, &s) == 0) && S_ISDIR(s.st_mode); } + +bool +virFileIsRegular(const char *path) +{ + struct stat s; + return (stat(path, &s) == 0) && S_ISREG(s.st_mode); +} + + /** * 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; - + VIR_FREE(path); if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) goto cleanup; + if (!virFileIsRegular(path)) + continue; + if (chown(path, uid, gid) < 0) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), @@ -3018,7 +3028,6 @@ int virFileChownFiles(const char *name, (unsigned int) gid); goto cleanup; } - VIR_FREE(path); } if (direrr < 0) diff --git a/src/util/virfile.h b/src/util/virfile.h index c7a32c30a8..59c14b97a6 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -194,6 +194,7 @@ off_t virFileLength(const char *path, int fd) ATTRIBUTE_NONNULL(1); bool virFileIsDir (const char *file) ATTRIBUTE_NONNULL(1); bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NOINLINE; bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1); +bool virFileIsRegular(const char *file) ATTRIBUTE_NONNULL(1); enum { VIR_FILE_SHFS_NFS = (1 << 0), -- 2.14.4
On Wed, Jun 06, 2018 at 12:37:06PM -0400, 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@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 17 +++++++++++++---- src/util/virfile.h | 1 + 3 files changed, 15 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@linux.vnet.ibm.com> --- 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+). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 06/06/2018 12:57 PM, Eric Blake wrote:
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@linux.vnet.ibm.com> --- 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+).
gnulib seems to implement readdir() but they also set d_type there and what you are showing above is unfortunately not in a header file but in fts.c :-( I guess for our purposes it would be good enough to have an equivalent of HAVE_STRUCT_DIRENT_D_TYPE set by test-compiling in configure?
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Stefan Berger