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

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

[Andrey Drobyshev]

v2 -> v3:
  * Patch 2/7:
    - ga_pipe_read_str() helper now returns -errno in case of an error
      during read from pipe, so that the caller may use it to set
      error_setg_errno();
    - ga_pipe_read_str() allocates +1 additional byte to make the
      string read from pipe null-terminated on every iteration;
  * Patch 6/7: patch is rewritten to completely get rid of fork()/exec()
    when suspending via sysfs, it now simply uses g_file_set_contents()
    (suggested by Daniel);
  * Patch 7/7: cosmetic change: removed unneeded brackets in an
    expression.

v2: https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg00147.html

Andrey Drobyshev (7):
  qga: guest-get-fsinfo: add optional 'total-bytes-root' field
  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: don't do fork()/exec() when suspending via sysfs
  qga/commands-posix: qmp_guest_set_user_password: use ga_run_command
    helper

 qga/commands-posix.c | 404 +++++++++++++++++++------------------------
 qga/commands-win32.c |   1 +
 qga/qapi-schema.json |  12 +-
 3 files changed, 193 insertions(+), 224 deletions(-)

-- 
2.39.3

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

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

[Markus Armbruster]

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

[Andrey Drobyshev]

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.

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

[Daniel P. Berrangé]

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

[Andrey Drobyshev]

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

[PATCH v3 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 | 150 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8207c4c47e..ad05630086 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -76,6 +76,156 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
     g_assert(rpid == pid);
 }
 
+static ssize_t ga_pipe_read_str(int fd[2], char **str)
+{
+    ssize_t n, len = 0;
+    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 {
+                len = -errno;
+                break;
+            }
+        }
+        *str = g_realloc(*str, len + n + 1);
+        memcpy(*str + len, buf, n);
+        len += n;
+        *str[len] = '\0';
+    }
+    close(fd[0]);
+    fd[0] = -1;
+
+    return len;
+}
+
+/*
+ * 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;
+    ssize_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;
+    }
+
+    len = ga_pipe_read_str(outfd, &str);
+    if (len < 0) {
+        error_setg_errno(errp, -len, "%s: cannot read from stdout/stderr pipe",
+                         action);
+        goto out;
+    }
+
+    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

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

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:41PM +0200, Andrey Drobyshev wrote:
> 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 | 150 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



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

[PATCH v3 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 ad05630086..d4025e0c1e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -108,7 +108,6 @@ static ssize_t ga_pipe_read_str(int fd[2], char **str)
  * 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)
 {
@@ -230,8 +229,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";
@@ -260,46 +257,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

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

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:42PM +0200, Andrey Drobyshev wrote:
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 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 d4025e0c1e..94b652d54e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -279,21 +279,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) {
@@ -324,37 +312,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

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

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:43PM +0200, Andrey Drobyshev wrote:
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 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 94b652d54e..610d225d30 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -736,8 +736,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;
@@ -746,42 +744,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

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

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:44PM +0200, Andrey Drobyshev wrote:
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs

[Andrey Drobyshev]

Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
logic") pm-utils logic is running in a separate child from the sysfs
logic.  Now when suspending via sysfs we don't really need to do that in
a separate process as we only need to perform one write to /sys/power/state.

Let's just use g_file_set_contents() to simplify things here.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 41 +++++------------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 610d225d30..e0ea377f65 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
 
 static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
 {
-    Error *local_err = NULL;
+    GError *local_gerr = 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;
-    }
-
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
+                             -1, &local_gerr)) {
+        error_setg(errp, "suspend: cannot write to '%s': %s",
+                   LINUX_SYS_STATE_FILE, local_gerr->message);
         return;
     }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to suspend");
-    }
-
 }
 
 static void guest_suspend(SuspendMode mode, Error **errp)
-- 
2.39.3

Re: [PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:45PM +0200, Andrey Drobyshev wrote:
> Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
> logic") pm-utils logic is running in a separate child from the sysfs
> logic.  Now when suspending via sysfs we don't really need to do that in
> a separate process as we only need to perform one write to /sys/power/state.
> 
> Let's just use g_file_set_contents() to simplify things here.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 41 +++++------------------------------------
>  1 file changed, 5 insertions(+), 36 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 610d225d30..e0ea377f65 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>  
>  static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>  {
> -    Error *local_err = NULL;
> +    GError *local_gerr = 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;
> -    }
> -
> -    ga_wait_child(pid, &status, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
> +                             -1, &local_gerr)) {
> +        error_setg(errp, "suspend: cannot write to '%s': %s",
> +                   LINUX_SYS_STATE_FILE, local_gerr->message);

You need to declare with "g_autoptr(GError) local_gerr = NULL" to
avoid a leak here.

>          return;
>      }
> -
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp, "child process has failed to suspend");
> -    }
> -
>  }
>  
>  static void guest_suspend(SuspendMode mode, Error **errp)
> -- 
> 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 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs

[Andrey Drobyshev]

On 3/19/24 20:02, Daniel P. Berrangé wrote:
> On Fri, Mar 15, 2024 at 02:29:45PM +0200, Andrey Drobyshev wrote:
>> Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
>> logic") pm-utils logic is running in a separate child from the sysfs
>> logic.  Now when suspending via sysfs we don't really need to do that in
>> a separate process as we only need to perform one write to /sys/power/state.
>>
>> Let's just use g_file_set_contents() to simplify things here.
>>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 41 +++++------------------------------------
>>  1 file changed, 5 insertions(+), 36 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 610d225d30..e0ea377f65 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>>  
>>  static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>>  {
>> -    Error *local_err = NULL;
>> +    GError *local_gerr = 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;
>> -    }
>> -
>> -    ga_wait_child(pid, &status, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> +    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
>> +                             -1, &local_gerr)) {
>> +        error_setg(errp, "suspend: cannot write to '%s': %s",
>> +                   LINUX_SYS_STATE_FILE, local_gerr->message);
> 
> You need to declare with "g_autoptr(GError) local_gerr = NULL" to
> avoid a leak here.
> 

Of course, thanks for noticing.

>>          return;
>>      }
>> -
>> -    if (WEXITSTATUS(status)) {
>> -        error_setg(errp, "child process has failed to suspend");
>> -    }
>> -
>>  }
>>  
>>  static void guest_suspend(SuspendMode mode, Error **errp)
>> -- 
>> 2.39.3
>>
> 
> With regards,
> Daniel

[PATCH v3 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 e0ea377f65..ffea69c3f0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2151,14 +2151,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) {
@@ -2169,95 +2163,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

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

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 02:29:46PM +0200, Andrey Drobyshev wrote:
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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