Re: [Qemu-devel] [PATCH v5.1 1/2] qga: add mountpoint usage info to GuestFilesystemInfo

[Eric Blake]

On 06/01/2018 11:57 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <address@hidden>
>
> This patch adds support for getting the usage of mounted
> filesystem.
> The usage of fs stored as used_bytes and total_bytes.
> It's very useful when we try to monitor guest's filesystem.
>
> Cc: Michael Roth <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: Daniel P. Berrangé <address@hidden>
> Signed-off-by: Chen Hanxiao <address@hidden>
> ---
> +++ b/qga/commands-posix.c
> @@ -46,6 +46,7 @@ extern char **environ;
>   #include <arpa/inet.h>
>   #include <sys/socket.h>
>   #include <net/if.h>
> +#include <sys/statvfs.h>
>   
>   #ifdef FIFREEZE
>   #define CONFIG_FSFREEZE
> @@ -1072,6 +1073,8 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct 
> FsMount *mount,
>                                                  Error **errp)
>   {
>       GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));

Because this is 0-initialized,

> +    struct statvfs buf;
> +    unsigned long used, nonroot_total, fr_size;
>       char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
>                                       mount->devmajor, mount->devminor);
>   
> @@ -1079,7 +1082,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
>       fs->type = g_strdup(mount->devtype);
>       build_guest_fsinfo_for_device(devpath, fs, errp);
>   
> +    if (statvfs(fs->mountpoint, &buf)) {
> +        fs->has_total_bytes = false;
> +        fs->has_used_bytes = false;

this branch is dead assignments.  You could simplify to just 'if 
(statvfs(...) == 0)' with no 'else' branch needed.

> +    } else {
> +        fr_size = buf.f_frsize;
> +        used = buf.f_blocks - buf.f_bfree;
> +        nonroot_total = used + buf.f_bavail;
> +        fs->used_bytes = used * fr_size;
> +        fs->total_bytes = nonroot_total * fr_size;
> +
> +        fs->has_total_bytes = true;
> +        fs->has_used_bytes = true;
> +    }
> +
>       g_free(devpath);
> +
>       return fs;
>   }
>   
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 17884c7c70..56ce2d08e2 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -846,6 +846,8 @@
>   # @name: disk name
>   # @mountpoint: mount point path
>   # @type: file system type string
> +# @used-bytes: file system used bytes (since 3.0)
> +# @total-bytes: nonroot file system total bytes (since 3.0)

s/nonroot/non-root/

>   # @disk: an array of disk hardware information that the volume lies on,
>   #        which may be empty if the disk type is not supported
>   #
> @@ -853,6 +855,7 @@
>   ##
>   { 'struct': 'GuestFilesystemInfo',
>     'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> +           '*used-bytes': 'uint64', '*total-bytes': 'uint64',
>              'disk': ['GuestDiskAddress']} }
>   
>   ##

Both those changes are minor, so at this point, I'm comfortable with:
Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org