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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/20/2016 11:01 PM, 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.
> 
> 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).

I almost wonder if it would be better to take both a path AND an fd,
with semantics of:

if fd == -1, use path alone - stat()
if fd >= 0, path should be non-NULL and describes the fd (for error
messages) - fstat()

For block devices, [f]stat() probably won't do what we want, where
lseek(SEEK_END) and/or ioctl() may be better.  But like you say, those
can be added later.

I definitely like the idea of having a nice wrapper function that we can
enhance later, to abstract out the logic so that it is not repeated in
the callers.

> ---
> New in "V2"
> 
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c       | 13 +++++++++++++
>  src/util/virfile.h       |  2 ++
>  3 files changed, 16 insertions(+)
> 
>  
> +
> +off_t
> +virFileLength(const char *file)
> +{
> +    struct stat s;
> +
> +    if (stat(file, &s) < 0)
> +        return -1;
> +
> +    return s.st_size;

Since we KNOW it won't work right on block devices, and we have the
stat() results, shouldn't this also check for S_ISREG(s.st_mode) and
return -1 if not?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux