On 2/28/24 09:55, Marc-André Lureau wrote: > [Вы нечасто получаете письма от marcandre.lur...@redhat.com. Узнайте, почему > это важно, по адресу https://aka.ms/LearnAboutSenderIdentification ] > > Hi > > On Tue, Feb 27, 2024 at 4:38 PM Andrey Drobyshev > <andrey.drobys...@virtuozzo.com> wrote: >> >> >> >> 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.drobys...@virtuozzo.com <mailto:andrey.drobys...@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 <y...@virtuozzo.com >>> <mailto:y...@virtuozzo.com>> >>> Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com >>> <mailto:andrey.drobys...@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? > > > Why break backward compatibility? Don't break other systems (win32) > when you propose a patch. > > QGA API aims to be cross-platform. Any system should be able to report > some kind of meaningful used and total disk space. I don't see much > reason to change that. > > If we need Posix-specific values reported by statvfs(), we can have > extra optional struct/fields. > > Fwiw, I find it more obscure to report statvfs values :) Perhaps we > should simply document better where those values are coming from, > instead of reporting more system-specific details. >
Agreed, keeping API compatible with Win version is a valid point. I've checked Win32 API page for GetDiskFreeSpaceExA(), and it seems TotalNumberOfBytes they return is exactly for the non-privileged user. So that's probably the root for such a design: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getdiskfreespaceexa Let me add an extra optional field then which we'd only fill on POSIX. We might call it 'total-bytes-root' to highlight the difference. I'd also follow your advice and document where those values come from in both POSIX and Win case. I'll send this in v2. Andrey