| Message ID | 20240226165642.807350-2-andrey.drobyshev@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga/commands-posix: replace code duplicating commands with a helper | expand |
Best Regards, Konstantin Kostiuk. On Mon, Feb 26, 2024 at 7:02 PM Andrey Drobyshev < 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> > Signed-off-by: Andrey Drobyshev <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(struct 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 Currently, we support both 'blacklist' and 'block-rpcs' command line options but the first one wrote a warning. @Marc-André Lureau <marcandre.lureau@redhat.com> @Philippe Mathieu-Daudé <philmd@linaro.org> What do you think about this? > +# > +# @avail-bytes: amount of free space in file system for unprivileged > +# users in bytes (since 8.3) > # > # @disk: an array of disk hardware information that the volume lies > # on, which may be empty if the disk type is not supported > @@ -1041,8 +1044,8 @@ > ## > { 'struct': 'GuestFilesystemInfo', > 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str', > - '*used-bytes': 'uint64', '*total-bytes': 'uint64', > - 'disk': ['GuestDiskAddress']} } > + '*total-bytes': 'uint64', '*free-bytes': 'uint64', > + '*avail-bytes': 'uint64', 'disk': ['GuestDiskAddress']} } > > ## > # @guest-get-fsinfo: > -- > 2.39.3 > > >
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? > > [...]
Hi On Tue, Feb 27, 2024 at 4:38 PM Andrey Drobyshev <andrey.drobyshev@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.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? 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.
On 2/28/24 09:55, Marc-André Lureau wrote: > [Вы нечасто получаете письма от marcandre.lureau@redhat.com. Узнайте, почему это важно, по адресу https://aka.ms/LearnAboutSenderIdentification ] > > Hi > > On Tue, Feb 27, 2024 at 4:38 PM Andrey Drobyshev > <andrey.drobyshev@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.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? > > > 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
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(struct 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) +# +# @avail-bytes: amount of free space in file system for unprivileged +# users in bytes (since 8.3) # # @disk: an array of disk hardware information that the volume lies # on, which may be empty if the disk type is not supported @@ -1041,8 +1044,8 @@ ## { 'struct': 'GuestFilesystemInfo', 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str', - '*used-bytes': 'uint64', '*total-bytes': 'uint64', - 'disk': ['GuestDiskAddress']} } + '*total-bytes': 'uint64', '*free-bytes': 'uint64', + '*avail-bytes': '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). 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> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> --- qga/commands-posix.c | 16 +++++++--------- qga/qapi-schema.json | 11 +++++++---- 2 files changed, 14 insertions(+), 13 deletions(-)