[PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field

Andrey Drobyshev posted 7 patches 1 year, 11 months ago
Maintainers: Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>
There is a newer version of this series
[PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
Posted by Andrey Drobyshev 1 year, 11 months ago
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) as
returned by statvfs(3).  While on Windows guests that's all we can get
with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
total file system size, as it's visible for root user.  Let's add an
optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
only be reported on POSIX and represent f_blocks value as returned by
statvfs(3).

While here, let's document better where those values come from in both
POSIX and Windows.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c |  2 ++
 qga/commands-win32.c |  1 +
 qga/qapi-schema.json | 12 +++++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26008db497..8207c4c47e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1569,8 +1569,10 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
         nonroot_total = used + buf.f_bavail;
         fs->used_bytes = used * fr_size;
         fs->total_bytes = nonroot_total * fr_size;
+        fs->total_bytes_root = buf.f_blocks * fr_size;
 
         fs->has_total_bytes = true;
+        fs->has_total_bytes_root = true;
         fs->has_used_bytes = true;
     }
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a1015757d8..9e820aad8d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1143,6 +1143,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
     fs = g_malloc(sizeof(*fs));
     fs->name = g_strdup(guid);
     fs->has_total_bytes = false;
+    fs->has_total_bytes_root = false;
     fs->has_used_bytes = false;
     if (len == 0) {
         fs->mountpoint = g_strdup("System Reserved");
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b8efe31897..093a5ab602 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1031,8 +1031,18 @@
 # @type: file system type string
 #
 # @used-bytes: file system used bytes (since 3.0)
+#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
+#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
+#       by GetDiskFreeSpaceEx()
 #
 # @total-bytes: non-root file system total bytes (since 3.0)
+#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
+#       statvfs(3)
+#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
+#
+# @total-bytes-root: total file system size in bytes (as visible for a
+#     priviledged user) (since 8.3)
+#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
 #
 # @disk: an array of disk hardware information that the volume lies
 #     on, which may be empty if the disk type is not supported
@@ -1042,7 +1052,7 @@
 { 'struct': 'GuestFilesystemInfo',
   'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
            '*used-bytes': 'uint64', '*total-bytes': 'uint64',
-           'disk': ['GuestDiskAddress']} }
+           '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
 
 ##
 # @guest-get-fsinfo:
-- 
2.39.3
Re: [PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
Posted by Daniel P. Berrangé 1 year, 10 months ago
On Fri, Mar 15, 2024 at 02:29:40PM +0200, Andrey Drobyshev 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) as
> returned by statvfs(3).  While on Windows guests that's all we can get
> with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
> total file system size, as it's visible for root user.  Let's add an
> optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
> only be reported on POSIX and represent f_blocks value as returned by
> statvfs(3).
> 
> While here, let's document better where those values come from in both
> POSIX and Windows.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c |  2 ++
>  qga/commands-win32.c |  1 +
>  qga/qapi-schema.json | 12 +++++++++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 26008db497..8207c4c47e 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1569,8 +1569,10 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
>          nonroot_total = used + buf.f_bavail;
>          fs->used_bytes = used * fr_size;
>          fs->total_bytes = nonroot_total * fr_size;
> +        fs->total_bytes_root = buf.f_blocks * fr_size;
>  
>          fs->has_total_bytes = true;
> +        fs->has_total_bytes_root = true;
>          fs->has_used_bytes = true;
>      }
>  
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index a1015757d8..9e820aad8d 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1143,6 +1143,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
>      fs = g_malloc(sizeof(*fs));
>      fs->name = g_strdup(guid);
>      fs->has_total_bytes = false;
> +    fs->has_total_bytes_root = false;

Can we use GetDiskSpaceInformationA to return this information
on Windows ? In contrast to GetDiskFreeSpaceExA(), the
DISK_SPACE_INFORMATION struct details both the real sizes
and the current user available sizes.

>      fs->has_used_bytes = false;
>      if (len == 0) {
>          fs->mountpoint = g_strdup("System Reserved");
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b8efe31897..093a5ab602 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1031,8 +1031,18 @@
>  # @type: file system type string
>  #
>  # @used-bytes: file system used bytes (since 3.0)
> +#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
> +#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
> +#       by GetDiskFreeSpaceEx()
>  #
>  # @total-bytes: non-root file system total bytes (since 3.0)
> +#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
> +#       statvfs(3)
> +#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
> +#
> +# @total-bytes-root: total file system size in bytes (as visible for a
> +#     priviledged user) (since 8.3)
> +#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)

I tend to wonder whether it is really a good idea to document
our specific implementation details in the public API

I might suggest

  @total-bytes: filesystem capacity in bytes for unprivileged users
  @total-bytes-root: filesystem capacity in bytes for privileged users

also should we call it 'total-bytes-privilegd', to avoid UNIX specific
terminology.

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

With 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 :|
Re: [PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
Posted by Andrey Drobyshev 1 year, 10 months ago
On 3/19/24 19:37, Daniel P. Berrangé wrote:
> On Fri, Mar 15, 2024 at 02:29:40PM +0200, Andrey Drobyshev 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) as
>> returned by statvfs(3).  While on Windows guests that's all we can get
>> with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
>> total file system size, as it's visible for root user.  Let's add an
>> optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
>> only be reported on POSIX and represent f_blocks value as returned by
>> statvfs(3).
>>
>> While here, let's document better where those values come from in both
>> POSIX and Windows.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c |  2 ++
>>  qga/commands-win32.c |  1 +
>>  qga/qapi-schema.json | 12 +++++++++++-
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 26008db497..8207c4c47e 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1569,8 +1569,10 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
>>          nonroot_total = used + buf.f_bavail;
>>          fs->used_bytes = used * fr_size;
>>          fs->total_bytes = nonroot_total * fr_size;
>> +        fs->total_bytes_root = buf.f_blocks * fr_size;
>>  
>>          fs->has_total_bytes = true;
>> +        fs->has_total_bytes_root = true;
>>          fs->has_used_bytes = true;
>>      }
>>  
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index a1015757d8..9e820aad8d 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -1143,6 +1143,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
>>      fs = g_malloc(sizeof(*fs));
>>      fs->name = g_strdup(guid);
>>      fs->has_total_bytes = false;
>> +    fs->has_total_bytes_root = false;
> 
> Can we use GetDiskSpaceInformationA to return this information
> on Windows ? In contrast to GetDiskFreeSpaceExA(), the
> DISK_SPACE_INFORMATION struct details both the real sizes
> and the current user available sizes.
> 

It seems that this API has only been included in mingw64 recently:
https://github.com/mingw-w64/mingw-w64/commit/66546556

Apparently since it happened there hasn't been a new release of mingw64,
so the latest version v11.0.1 still doesn't have it.  So I guess we have
no choice but to leave Win fields as-is for now and switch to the new
API later.

>>      fs->has_used_bytes = false;
>>      if (len == 0) {
>>          fs->mountpoint = g_strdup("System Reserved");
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index b8efe31897..093a5ab602 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -1031,8 +1031,18 @@
>>  # @type: file system type string
>>  #
>>  # @used-bytes: file system used bytes (since 3.0)
>> +#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
>> +#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
>> +#       by GetDiskFreeSpaceEx()
>>  #
>>  # @total-bytes: non-root file system total bytes (since 3.0)
>> +#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
>> +#       statvfs(3)
>> +#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
>> +#
>> +# @total-bytes-root: total file system size in bytes (as visible for a
>> +#     priviledged user) (since 8.3)
>> +#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
> 
> I tend to wonder whether it is really a good idea to document
> our specific implementation details in the public API
> 
> I might suggest
> 
>   @total-bytes: filesystem capacity in bytes for unprivileged users
>   @total-bytes-root: filesystem capacity in bytes for privileged users
>

My initial intent was to get rid of the necessity to dig into the
sources in order to understand what those values mean.  But since we're
discussing changes in the implementation, I guess it is wise not to
mention those details indeed.

> also should we call it 'total-bytes-privilegd', to avoid UNIX specific
> terminology.
> 

Agreed.

>>  #
>>  # @disk: an array of disk hardware information that the volume lies
>>  #     on, which may be empty if the disk type is not supported
>> @@ -1042,7 +1052,7 @@
>>  { 'struct': 'GuestFilesystemInfo',
>>    'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
>>             '*used-bytes': 'uint64', '*total-bytes': 'uint64',
>> -           'disk': ['GuestDiskAddress']} }
>> +           '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
>>  
>>  ##
>>  # @guest-get-fsinfo:
>> -- 
>> 2.39.3
>>
> 
> With regards,
> Daniel


Re: [PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
Posted by Markus Armbruster 1 year, 11 months ago
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> writes:

> 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) as
> returned by statvfs(3).  While on Windows guests that's all we can get
> with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
> total file system size, as it's visible for root user.  Let's add an
> optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
> only be reported on POSIX and represent f_blocks value as returned by
> statvfs(3).
>
> While here, let's document better where those values come from in both
> POSIX and Windows.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

[...]

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b8efe31897..093a5ab602 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1031,8 +1031,18 @@
>  # @type: file system type string
>  #
>  # @used-bytes: file system used bytes (since 3.0)
> +#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
> +#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
> +#       by GetDiskFreeSpaceEx()
>  #
>  # @total-bytes: non-root file system total bytes (since 3.0)
> +#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
> +#       statvfs(3)
> +#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
> +#
> +# @total-bytes-root: total file system size in bytes (as visible for a
> +#     priviledged user) (since 8.3)

privileged

> +#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
>  #
>  # @disk: an array of disk hardware information that the volume lies
>  #     on, which may be empty if the disk type is not supported
> @@ -1042,7 +1052,7 @@
>  { 'struct': 'GuestFilesystemInfo',
>    'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
>             '*used-bytes': 'uint64', '*total-bytes': 'uint64',
> -           'disk': ['GuestDiskAddress']} }
> +           '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
>  
>  ##
>  # @guest-get-fsinfo:

Fails to build the manual:

    qga/qapi-schema.json:1019:Unexpected indentation.

To fix, add blank lines before the lists, like this:

   # @used-bytes: file system used bytes (since 3.0)
   #
   #     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by
   #       statvfs(3)
   #     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as
   #       returned by GetDiskFreeSpaceEx()
   #
   # @total-bytes: non-root file system total bytes (since 3.0)
   #
   #     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
   #       statvfs(3)
   #     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
   #
   # @total-bytes-root: total file system size in bytes (as visible for a
   #     privileged user) (since 8.3)
   #
   #     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
   #

Yes, reST can be quite annoying.
Re: [PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
Posted by Andrey Drobyshev 1 year, 11 months ago
On 3/15/24 15:44, Markus Armbruster wrote:
> [?? ??????? ????????? ?????? ?? armbru@redhat.com. ???????, ?????? ??? ?????, ?? ?????? https://aka.ms/LearnAboutSenderIdentification ]
> 
> Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> writes:
> 
>> 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) as
>> returned by statvfs(3).  While on Windows guests that's all we can get
>> with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
>> total file system size, as it's visible for root user.  Let's add an
>> optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
>> only be reported on POSIX and represent f_blocks value as returned by
>> statvfs(3).
>>
>> While here, let's document better where those values come from in both
>> POSIX and Windows.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> 
> [...]
> 
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index b8efe31897..093a5ab602 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -1031,8 +1031,18 @@
>>  # @type: file system type string
>>  #
>>  # @used-bytes: file system used bytes (since 3.0)
>> +#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
>> +#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
>> +#       by GetDiskFreeSpaceEx()
>>  #
>>  # @total-bytes: non-root file system total bytes (since 3.0)
>> +#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
>> +#       statvfs(3)
>> +#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
>> +#
>> +# @total-bytes-root: total file system size in bytes (as visible for a
>> +#     priviledged user) (since 8.3)
> 
> privileged
> 
>> +#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
>>  #
>>  # @disk: an array of disk hardware information that the volume lies
>>  #     on, which may be empty if the disk type is not supported
>> @@ -1042,7 +1052,7 @@
>>  { 'struct': 'GuestFilesystemInfo',
>>    'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
>>             '*used-bytes': 'uint64', '*total-bytes': 'uint64',
>> -           'disk': ['GuestDiskAddress']} }
>> +           '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
>>
>>  ##
>>  # @guest-get-fsinfo:
> 
> Fails to build the manual:
> 
>     qga/qapi-schema.json:1019:Unexpected indentation.
> 
> To fix, add blank lines before the lists, like this:
> 
>    # @used-bytes: file system used bytes (since 3.0)
>    #
>    #     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by
>    #       statvfs(3)
>    #     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as
>    #       returned by GetDiskFreeSpaceEx()
>    #
>    # @total-bytes: non-root file system total bytes (since 3.0)
>    #
>    #     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
>    #       statvfs(3)
>    #     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
>    #
>    # @total-bytes-root: total file system size in bytes (as visible for a
>    #     privileged user) (since 8.3)
>    #
>    #     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
>    #
> 
> Yes, reST can be quite annoying.
> 

For some reason in my environment build doesn't fail and file
build/docs/qemu-ga-ref.7 is produced.  However lists aren't indeed
formatted properly.  I'll wait for other patches to be reviewed and fix
that in v4.  Thank you for noticing.