Re: [PATCH 1/7] qga/commands-posix: return fsinfo values directly as reported by statvfs

[Andrey Drobyshev]

On 2/26/24 20:50, Konstantin Kostiuk wrote:
> 
> Best Regards,
> Konstantin Kostiuk.
> 
> 
> On Mon, Feb 26, 2024 at 7:02 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com <mailto:andrey.drobyshev@virtuozzo.com>>
> wrote:
> 
>     Since the commit 25b5ff1a86 ("qga: add mountpoint usage info to
>     GuestFilesystemInfo") we have 2 values reported in guest-get-fsinfo:
>     used = (f_blocks - f_bfree), total = (f_blocks - f_bfree + f_bavail).
>     These calculations might be obscure for the end user and require one to
>     actually get into QGA source to understand how they're obtained. Let's
>     just report the values f_blocks, f_bfree, f_bavail (in bytes) from
>     statvfs() as they are, letting the user decide how to process them
>     further.
> 
>     Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com
>     <mailto:yur@virtuozzo.com>>
>     Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com
>     <mailto:andrey.drobyshev@virtuozzo.com>>
>     ---
>      qga/commands-posix.c | 16 +++++++---------
>      qga/qapi-schema.json | 11 +++++++----
>      2 files changed, 14 insertions(+), 13 deletions(-)
> 
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index 26008db497..752ef509d0 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -1554,8 +1554,7 @@ static GuestFilesystemInfo
>     *build_guest_fsinfo(strua5a0239ce5ct FsMount *mount,
>                                                     Error **errp)
>      {
>          GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
>     -    struct statvfs buf;
>     -    unsigned long used, nonroot_total, fr_size;
>     +    struct statvfs st;
>          char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
>                                          mount->devmajor, mount->devminor);
> 
>     @@ -1563,15 +1562,14 @@ 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) == 0) {
>     -        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;
>     +    if (statvfs(fs->mountpoint, &st) == 0) {
>     +        fs->total_bytes = st.f_blocks * st.f_frsize;
>     +        fs->free_bytes = st.f_bfree * st.f_frsize;
>     +        fs->avail_bytes = st.f_bavail * st.f_frsize;
> 
>              fs->has_total_bytes = true;
>     -        fs->has_used_bytes = true;
>     +        fs->has_free_bytes = true;
>     +        fs->has_avail_bytes = true;
>          }
> 
>          g_free(devpath);
>     diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>     index b8efe31897..1cce3c1df5 100644
>     --- a/qga/qapi-schema.json
>     +++ b/qga/qapi-schema.json
>     @@ -1030,9 +1030,12 @@
>      #
>      # @type: file system type string
>      #
>     -# @used-bytes: file system used bytes (since 3.0)
>     +# @total-bytes: total file system size in bytes (since 8.3)
>      #
>     -# @total-bytes: non-root file system total bytes (since 3.0)
>     +# @free-bytes: amount of free space in file system in bytes (since 8.3)
> 
> 
> I don't agree with this as it breaks backward compatibility. If we want
> to get
> these changes we should release a new version with both old and new fields
> and mark old as deprecated to get a time for everyone who uses this
> API updates its solutions.
> 
> A similar thing was with replacing the 'blacklist' command line.
> https://gitlab.com/qemu-project/qemu/-/commit/582a098e6ca00dd42f317dad8affd13e5a20bc42
>  
> <https://gitlab.com/qemu-project/qemu/-/commit/582a098e6ca00dd42f317dad8affd13e5a20bc42>
> Currently, we support both 'blacklist' and 'block-rpcs' command line options
> but the first one wrote a warning.
>

I agree that marking the old values as deprecated does make sense.
Although my original intent with this patch is to make more sense of the
existing names (e.g. total-bytes to indicate true fs size instead of
just non-root fs).  If so, we'd eventually have to replace the original
total-bytes value with the one having new semantics.  Or we could rename
the existing value to smth like "total-bytes-nonroot".  But either way
breaks backward compatibility after all.  How would you suggest to
resolve it?

Andrey

> @Marc-André Lureau <mailto:marcandre.lureau@redhat.com> @Philippe
> Mathieu-Daudé <mailto:philmd@linaro.org> 
> What do you think about this?
>  
> [...]