[PATCH 0/7] qga/commands-posix: replace code duplicating commands with a helper

[PATCH 0/7] qga/commands-posix: replace code duplicating commands with a helper

[Andrey Drobyshev]

This series simply replaces repeating fork()/exec() pattern with a
separate helper to avoid code duplication.  It's easier to setup and use
than g_spawn_async_with_pipes() (which we'd need since some commands require
input).  While here, also make qmp_guest_get_fsinfo return more
straightforward values.

Andrey Drobyshev (7):
  qga/commands-posix: return fsinfo values directly as reported by
    statvfs
  qga: introduce ga_run_command() helper for guest cmd execution
  qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper
  qga/commands-posix: qmp_guest_set_time: use ga_run_command helper
  qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper
  qga/commands-posix: use ga_run_command helper when suspending via
    sysfs
  qga/commands-posix: qmp_guest_set_user_password: use ga_run_command
    helper

 qga/commands-posix.c | 402 +++++++++++++++++++------------------------
 qga/qapi-schema.json |  11 +-
 2 files changed, 181 insertions(+), 232 deletions(-)

-- 
2.39.3

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

[Andrey Drobyshev]

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)
+#
+# @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

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

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 4339 bytes --]

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 6065 bytes --]

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?
>  
> [...]

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

[Marc-André Lureau]

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.

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

[Andrey Drobyshev]

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

[PATCH 2/7] qga: introduce ga_run_command() helper for guest cmd execution

[Andrey Drobyshev]

When executing guest commands in *nix environment, we repeat the same
fork/exec pattern multiple times.  Let's just separate it into a single
helper which would also be able to feed input data into the launched
process' stdin.  This way we can avoid code duplication.

To keep the history more bisectable, let's replace qmp commands
implementations one by one.  Also add G_GNUC_UNUSED attribute to the
helper and remove it in the next commit.

Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 140 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 752ef509d0..510b902b1a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -76,6 +76,146 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
     g_assert(rpid == pid);
 }
 
+static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
+{
+    ssize_t n;
+    char buf[1024];
+    close(fd[1]);
+    fd[1] = -1;
+    while ((n = read(fd[0], buf, sizeof(buf))) != 0) {
+        if (n < 0) {
+            if (errno == EINTR) {
+                continue;
+            } else {
+                break;
+            }
+        }
+        *str = g_realloc(*str, *len + n);
+        memcpy(*str + *len, buf, n);
+        *len += n;
+    }
+    close(fd[0]);
+    fd[0] = -1;
+}
+
+/*
+ * Helper to run command with input/output redirection,
+ * sending string to stdin and taking error message from
+ * stdout/err.
+ */
+G_GNUC_UNUSED
+static int ga_run_command(const char *argv[], const char *in_str,
+                          const char *action, Error **errp)
+{
+    pid_t pid;
+    int status;
+    int retcode = -1;
+    int infd[2] = { -1, -1 };
+    int outfd[2] = { -1, -1 };
+    char *str = NULL;
+    size_t len = 0;
+
+    if ((in_str && !g_unix_open_pipe(infd, FD_CLOEXEC, NULL)) ||
+        !g_unix_open_pipe(outfd, FD_CLOEXEC, NULL)) {
+        error_setg(errp, "cannot create pipe FDs");
+        goto out;
+    }
+
+    pid = fork();
+    if (pid == 0) {
+        char *cherr = NULL;
+
+        setsid();
+
+        if (in_str) {
+            /* Redirect stdin to infd. */
+            close(infd[1]);
+            dup2(infd[0], 0);
+            close(infd[0]);
+        } else {
+            reopen_fd_to_null(0);
+        }
+
+        /* Redirect stdout/stderr to outfd. */
+        close(outfd[0]);
+        dup2(outfd[1], 1);
+        dup2(outfd[1], 2);
+        close(outfd[1]);
+
+        execvp(argv[0], (char *const *)argv);
+
+        /* Write the cause of failed exec to pipe for the parent to read it. */
+        cherr = g_strdup_printf("failed to exec '%s'", argv[0]);
+        perror(cherr);
+        g_free(cherr);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        goto out;
+    }
+
+    if (in_str) {
+        close(infd[0]);
+        infd[0] = -1;
+        if (qemu_write_full(infd[1], in_str, strlen(in_str)) !=
+                strlen(in_str)) {
+            error_setg_errno(errp, errno, "%s: cannot write to stdin pipe",
+                             action);
+            goto out;
+        }
+        close(infd[1]);
+        infd[1] = -1;
+    }
+
+    ga_pipe_read_str(outfd, &str, &len);
+
+    ga_wait_child(pid, &status, errp);
+    if (*errp) {
+        goto out;
+    }
+
+    if (!WIFEXITED(status)) {
+        if (len) {
+            error_setg(errp, "child process has terminated abnormally: %s",
+                       str);
+        } else {
+            error_setg(errp, "child process has terminated abnormally");
+        }
+        goto out;
+    }
+
+    retcode = WEXITSTATUS(status);
+
+    if (WEXITSTATUS(status)) {
+        if (len) {
+            error_setg(errp, "child process has failed to %s: %s",
+                       action, str);
+        } else {
+            error_setg(errp, "child process has failed to %s: exit status %d",
+                       action, WEXITSTATUS(status));
+        }
+        goto out;
+    }
+
+out:
+    g_free(str);
+
+    if (infd[0] != -1) {
+        close(infd[0]);
+    }
+    if (infd[1] != -1) {
+        close(infd[1]);
+    }
+    if (outfd[0] != -1) {
+        close(outfd[0]);
+    }
+    if (outfd[1] != -1) {
+        close(outfd[1]);
+    }
+
+    return retcode;
+}
+
 void qmp_guest_shutdown(const char *mode, Error **errp)
 {
     const char *shutdown_flag;
-- 
2.39.3

[PATCH 3/7] qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper

[Andrey Drobyshev]

Also remove the G_GNUC_UNUSED attribute added in the previous commit from
the helper.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 39 ++++++---------------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 510b902b1a..4b64716bab 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -103,7 +103,6 @@ static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
  * sending string to stdin and taking error message from
  * stdout/err.
  */
-G_GNUC_UNUSED
 static int ga_run_command(const char *argv[], const char *in_str,
                           const char *action, Error **errp)
 {
@@ -220,8 +219,6 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
 {
     const char *shutdown_flag;
     Error *local_err = NULL;
-    pid_t pid;
-    int status;
 
 #ifdef CONFIG_SOLARIS
     const char *powerdown_flag = "-i5";
@@ -250,46 +247,22 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
         return;
     }
 
-    pid = fork();
-    if (pid == 0) {
-        /* child, start the shutdown */
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
+    const char *argv[] = {"/sbin/shutdown",
 #ifdef CONFIG_SOLARIS
-        execl("/sbin/shutdown", "shutdown", shutdown_flag, "-g0", "-y",
-              "hypervisor initiated shutdown", (char *)NULL);
+                          shutdown_flag, "-g0", "-y",
 #elif defined(CONFIG_BSD)
-        execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
-               "hypervisor initiated shutdown", (char *)NULL);
+                          shutdown_flag, "+0",
 #else
-        execl("/sbin/shutdown", "shutdown", "-h", shutdown_flag, "+0",
-               "hypervisor initiated shutdown", (char *)NULL);
+                          "-h", shutdown_flag, "+0",
 #endif
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+                          "hypervisor initiated shutdown", (char *) NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "shutdown", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        return;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to shutdown");
-        return;
-    }
-
     /* succeeded */
 }
 
-- 
2.39.3

[PATCH 4/7] qga/commands-posix: qmp_guest_set_time: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of "/sbin/hwclock", the
exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 4b64716bab..b704006c98 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -269,21 +269,9 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
 {
     int ret;
-    int status;
-    pid_t pid;
     Error *local_err = NULL;
     struct timeval tv;
-    static const char hwclock_path[] = "/sbin/hwclock";
-    static int hwclock_available = -1;
-
-    if (hwclock_available < 0) {
-        hwclock_available = (access(hwclock_path, X_OK) == 0);
-    }
-
-    if (!hwclock_available) {
-        error_setg(errp, QERR_UNSUPPORTED);
-        return;
-    }
+    const char *argv[] = {"/sbin/hwclock", has_time ? "-w" : "-s", NULL};
 
     /* If user has passed a time, validate and set it. */
     if (has_time) {
@@ -314,37 +302,12 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
      * just need to synchronize the hardware clock. However, if no time was
      * passed, user is requesting the opposite: set the system time from the
      * hardware clock (RTC). */
-    pid = fork();
-    if (pid == 0) {
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        /* Use '/sbin/hwclock -w' to set RTC from the system time,
-         * or '/sbin/hwclock -s' to set the system time from RTC. */
-        execl(hwclock_path, "hwclock", has_time ? "-w" : "-s", NULL);
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
-
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "set hardware clock to system time",
+                   &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        return;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "hwclock failed to set hardware clock to system time");
-        return;
-    }
 }
 
 typedef enum {
-- 
2.39.3

[PATCH 5/7] qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of the hook executable, as the
exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index b704006c98..6baaabc29e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -726,8 +726,6 @@ static const char *fsfreeze_hook_arg_string[] = {
 
 static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
 {
-    int status;
-    pid_t pid;
     const char *hook;
     const char *arg_str = fsfreeze_hook_arg_string[arg];
     Error *local_err = NULL;
@@ -736,42 +734,15 @@ static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
     if (!hook) {
         return;
     }
-    if (access(hook, X_OK) != 0) {
-        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'", hook);
-        return;
-    }
 
-    slog("executing fsfreeze hook with arg '%s'", arg_str);
-    pid = fork();
-    if (pid == 0) {
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        execl(hook, hook, arg_str, NULL);
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+    const char *argv[] = {hook, arg_str, NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    slog("executing fsfreeze hook with arg '%s'", arg_str);
+    ga_run_command(argv, NULL, "execute fsfreeze hook", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "fsfreeze hook has terminated abnormally");
-        return;
-    }
-
-    status = WEXITSTATUS(status);
-    if (status) {
-        error_setg(errp, "fsfreeze hook has failed with status %d", status);
-        return;
-    }
 }
 
 /*
-- 
2.39.3

[PATCH 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs

[Andrey Drobyshev]

We replace the direct call to open() with a "sh -c 'echo ...'" call, so
that it becomes an executable command.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6baaabc29e..599f59a323 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1917,49 +1917,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
     Error *local_err = NULL;
     const char *sysfile_strs[3] = {"disk", "mem", NULL};
     const char *sysfile_str = sysfile_strs[mode];
-    pid_t pid;
-    int status;
 
     if (!sysfile_str) {
         error_setg(errp, "unknown guest suspend mode");
         return;
     }
 
-    pid = fork();
-    if (!pid) {
-        /* child */
-        int fd;
-
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
-        if (fd < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        _exit(EXIT_SUCCESS);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+    g_autofree char *echo_cmd = g_strdup_printf(
+        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
+    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "suspend", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to suspend");
-    }
-
 }
 
 static void guest_suspend(SuspendMode mode, Error **errp)
-- 
2.39.3

[PATCH 7/7] qga/commands-posix: qmp_guest_set_user_password: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of the "chpasswd", "pw"
executables, as the exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 96 ++++++--------------------------------------
 1 file changed, 13 insertions(+), 83 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 599f59a323..7f39636417 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2140,14 +2140,8 @@ void qmp_guest_set_user_password(const char *username,
                                  Error **errp)
 {
     Error *local_err = NULL;
-    char *passwd_path = NULL;
-    pid_t pid;
-    int status;
-    int datafd[2] = { -1, -1 };
-    char *rawpasswddata = NULL;
+    g_autofree char *rawpasswddata = NULL;
     size_t rawpasswdlen;
-    char *chpasswddata = NULL;
-    size_t chpasswdlen;
 
     rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
     if (!rawpasswddata) {
@@ -2158,95 +2152,31 @@ void qmp_guest_set_user_password(const char *username,
 
     if (strchr(rawpasswddata, '\n')) {
         error_setg(errp, "forbidden characters in raw password");
-        goto out;
+        return;
     }
 
     if (strchr(username, '\n') ||
         strchr(username, ':')) {
         error_setg(errp, "forbidden characters in username");
-        goto out;
+        return;
     }
 
 #ifdef __FreeBSD__
-    chpasswddata = g_strdup(rawpasswddata);
-    passwd_path = g_find_program_in_path("pw");
+    g_autofree char *chpasswdata = g_strdup(rawpasswddata);
+    const char *crypt_flag = (crypted) ? "-H" : "-h";
+    const char *argv[] = {"pw", "usermod", "-n", username,
+                          crypt_flag, "0", NULL};
 #else
-    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
-    passwd_path = g_find_program_in_path("chpasswd");
+    g_autofree char *chpasswddata = g_strdup_printf("%s:%s\n", username,
+                                                    rawpasswddata);
+    const char *crypt_flag = (crypted) ? "-e" : NULL;
+    const char *argv[] = {"chpasswd", crypt_flag, NULL};
 #endif
 
-    chpasswdlen = strlen(chpasswddata);
-
-    if (!passwd_path) {
-        error_setg(errp, "cannot find 'passwd' program in PATH");
-        goto out;
-    }
-
-    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
-        error_setg(errp, "cannot create pipe FDs");
-        goto out;
-    }
-
-    pid = fork();
-    if (pid == 0) {
-        close(datafd[1]);
-        /* child */
-        setsid();
-        dup2(datafd[0], 0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-#ifdef __FreeBSD__
-        const char *h_arg;
-        h_arg = (crypted) ? "-H" : "-h";
-        execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0", NULL);
-#else
-        if (crypted) {
-            execl(passwd_path, "chpasswd", "-e", NULL);
-        } else {
-            execl(passwd_path, "chpasswd", NULL);
-        }
-#endif
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
-    }
-    close(datafd[0]);
-    datafd[0] = -1;
-
-    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
-        error_setg_errno(errp, errno, "cannot write new account password");
-        goto out;
-    }
-    close(datafd[1]);
-    datafd[1] = -1;
-
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, chpasswddata, "set user password", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        goto out;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to set user password");
-        goto out;
-    }
-
-out:
-    g_free(chpasswddata);
-    g_free(rawpasswddata);
-    g_free(passwd_path);
-    if (datafd[0] != -1) {
-        close(datafd[0]);
-    }
-    if (datafd[1] != -1) {
-        close(datafd[1]);
+        return;
     }
 }
 #else /* __linux__ || __FreeBSD__ */
-- 
2.39.3