util: Implement and use virFileIsRegular() rather than d_type

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

Posted by Stefan Berger 7 years, 8 months ago

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

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

Posted by Daniel P. Berrangé 7 years, 8 months ago

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

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

Posted by Stefan Berger 7 years, 8 months ago

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

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

Posted by Eric Blake 7 years, 8 months ago

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

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

Posted by Stefan Berger 7 years, 8 months ago

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