| Message ID | 20240315122946.39168-2-andrey.drobyshev@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga/commands-posix: replace code duplicating commands with a helper | expand |
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.
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.
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
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
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:
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(-)