Re: [libvirt] [PATCH v2 1/4] util: new function virFileLength()

On Mon, 2016-11-21 at 00:01 -0500, Laine Stump wrote: > This new function just calls stat() and returns st_size (or -1 if > there is an error). We may decide we want this function to be more > complex, and handle things like block devices - this is a placeholder > (that works) for any more complicated funtion. s/funtion/function/ > NB: virFileLength() takes a path rather than an fd because it needs to > be called for files that can't be opened (due to permissions). > --- > New in "V2" > > src/libvirt_private.syms | 1 + > src/util/virfile.c | 13 +++++++++++++ > src/util/virfile.h | 2 ++ > 3 files changed, 16 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index ac6a1e1..1c0b912 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1578,6 +1578,7 @@ virFileIsLink; > virFileIsMountPoint; > virFileIsSharedFS; > virFileIsSharedFSType; > +virFileLength; > virFileLinkPointsTo; > virFileLock; > virFileLoopDeviceAssociate; > diff --git a/src/util/virfile.c b/src/util/virfile.c > index a45279a..11b6027 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -1735,6 +1735,19 @@ virFileActivateDirOverride(const char *argv0) > } > } > > + > +off_t > +virFileLength(const char *file) I find the lack of documentation disturbing. > +{ > + struct stat s; > + > + if (stat(file, &s) < 0) > + return -1; > + > + return s.st_size; > +} > + > + > bool > virFileIsDir(const char *path) > { > diff --git a/src/util/virfile.h b/src/util/virfile.h > index b4ae6ea..a0c646d 100644 > --- a/src/util/virfile.h > +++ b/src/util/virfile.h > @@ -179,6 +179,8 @@ char *virFileFindResourceFull(const char *filename, > void virFileActivateDirOverride(const char *argv0) > ATTRIBUTE_NONNULL(1); > > +off_t virFileLength(const char *file) ATTRIBUTE_NONNULL(1); > + > bool virFileIsDir (const char *file) ATTRIBUTE_NONNULL(1); How did a space end up here? Weird :) > bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1); > bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1); As you mention in the commit message, this simple implementation might not handle all cases; on the other hand, its addition to virFile might encourage others to use it. So I guess my question would be: is adding this function, in its current form, worth it? Would it be better to just call stat() in qemuDomainDeviceCalculatePCIConnectFlags(), and replace that later with a call to a more fleshed-out virFileLength() that can be used not just in that specific spot, but hopefully in a bunch other places?