src/libvirt_private.syms | 1 + src/util/virfile.c | 17 +++++++++++++---- src/util/virfile.h | 1 + 3 files changed, 15 insertions(+), 4 deletions(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/06/2018 12:48 PM, Daniel P. Berrangé wrote: > Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> Thanks. I pushed this. Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.