[PATCH 0/3] error: Eliminate QERR_QGA_COMMAND_FAILED

[PATCH 0/3] error: Eliminate QERR_QGA_COMMAND_FAILED

[Markus Armbruster]

Markus Armbruster (3):
  qga-win32: Improve guest-set-user-password, guest-file-open errors
  qga: Shorten several error messages
  qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop

 include/qapi/qmp/qerror.h |  3 ---
 qga/commands-win32.c      | 41 ++++++++++++++++-----------------------
 qga/commands.c            |  5 ++---
 3 files changed, 19 insertions(+), 30 deletions(-)

-- 
2.45.0

[PATCH 1/3] qga-win32: Improve guest-set-user-password, guest-file-open errors

[Markus Armbruster]

When guest-set-user-password's argument @password can't be converted
from UTF-8 to UTF-16, we report something like

    Guest agent command failed, error was 'Invalid sequence in conversion input'

Improve this to

    can't convert 'password' to UTF-16: Invalid sequence in conversion input

Likewise for argument @username, and guest-file-open argument @path,
even though I'm not sure you can actually get invalid input past the
QMP core there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6fee0e1e6f..ed31077457 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -217,6 +217,9 @@ int64_t qmp_guest_file_open(const char *path, const char *mode, Error **errp)
 
     w_path = g_utf8_to_utf16(path, -1, NULL, NULL, &gerr);
     if (!w_path) {
+        error_setg(errp, "can't convert 'path' to UTF-16: %s",
+                   gerr->message);
+        g_error_free(gerr);
         goto done;
     }
 
@@ -244,10 +247,6 @@ int64_t qmp_guest_file_open(const char *path, const char *mode, Error **errp)
     slog("guest-file-open, handle: % " PRId64, fd);
 
 done:
-    if (gerr) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
-        g_error_free(gerr);
-    }
     g_free(w_path);
     return fd;
 }
@@ -1946,11 +1945,17 @@ void qmp_guest_set_user_password(const char *username,
 
     user = g_utf8_to_utf16(username, -1, NULL, NULL, &gerr);
     if (!user) {
+        error_setg(errp, "can't convert 'username' to UTF-16: %s",
+                   gerr->message);
+        g_error_free(gerr);
         goto done;
     }
 
     wpass = g_utf8_to_utf16(rawpasswddata, -1, NULL, NULL, &gerr);
     if (!wpass) {
+        error_setg(errp, "can't convert 'password' to UTF-16: %s",
+                   gerr->message);
+        g_error_free(gerr);
         goto done;
     }
 
@@ -1966,10 +1971,6 @@ void qmp_guest_set_user_password(const char *username,
     }
 
 done:
-    if (gerr) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
-        g_error_free(gerr);
-    }
     g_free(user);
     g_free(wpass);
     g_free(rawpasswddata);
-- 
2.45.0

Re: [PATCH 1/3] qga-win32: Improve guest-set-user-password, guest-file-open errors

[Philippe Mathieu-Daudé]

On 14/5/24 12:58, Markus Armbruster wrote:
> When guest-set-user-password's argument @password can't be converted
> from UTF-8 to UTF-16, we report something like
> 
>      Guest agent command failed, error was 'Invalid sequence in conversion input'
> 
> Improve this to
> 
>      can't convert 'password' to UTF-16: Invalid sequence in conversion input
> 
> Likewise for argument @username, and guest-file-open argument @path,
> even though I'm not sure you can actually get invalid input past the
> QMP core there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] qga-win32: Improve guest-set-user-password, guest-file-open errors

[Konstantin Kostiuk]

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

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>



On Tue, May 14, 2024 at 2:03 PM Markus Armbruster <armbru@redhat.com> wrote:

> When guest-set-user-password's argument @password can't be converted
> from UTF-8 to UTF-16, we report something like
>
>     Guest agent command failed, error was 'Invalid sequence in conversion
> input'
>
> Improve this to
>
>     can't convert 'password' to UTF-16: Invalid sequence in conversion
> input
>
> Likewise for argument @username, and guest-file-open argument @path,
> even though I'm not sure you can actually get invalid input past the
> QMP core there.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qga/commands-win32.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6fee0e1e6f..ed31077457 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -217,6 +217,9 @@ int64_t qmp_guest_file_open(const char *path, const
> char *mode, Error **errp)
>
>      w_path = g_utf8_to_utf16(path, -1, NULL, NULL, &gerr);
>      if (!w_path) {
> +        error_setg(errp, "can't convert 'path' to UTF-16: %s",
> +                   gerr->message);
> +        g_error_free(gerr);
>          goto done;
>      }
>
> @@ -244,10 +247,6 @@ int64_t qmp_guest_file_open(const char *path, const
> char *mode, Error **errp)
>      slog("guest-file-open, handle: % " PRId64, fd);
>
>  done:
> -    if (gerr) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> -        g_error_free(gerr);
> -    }
>      g_free(w_path);
>      return fd;
>  }
> @@ -1946,11 +1945,17 @@ void qmp_guest_set_user_password(const char
> *username,
>
>      user = g_utf8_to_utf16(username, -1, NULL, NULL, &gerr);
>      if (!user) {
> +        error_setg(errp, "can't convert 'username' to UTF-16: %s",
> +                   gerr->message);
> +        g_error_free(gerr);
>          goto done;
>      }
>
>      wpass = g_utf8_to_utf16(rawpasswddata, -1, NULL, NULL, &gerr);
>      if (!wpass) {
> +        error_setg(errp, "can't convert 'password' to UTF-16: %s",
> +                   gerr->message);
> +        g_error_free(gerr);
>          goto done;
>      }
>
> @@ -1966,10 +1971,6 @@ void qmp_guest_set_user_password(const char
> *username,
>      }
>
>  done:
> -    if (gerr) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> -        g_error_free(gerr);
> -    }
>      g_free(user);
>      g_free(wpass);
>      g_free(rawpasswddata);
> --
> 2.45.0
>
>

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

[PATCH 2/3] qga: Shorten several error messages

[Markus Armbruster]

Some, but not all error messages are of the form

    Guest agent command failed, error was '<actual error message>'

For instance, command guest-exec can fail with an error message like

    Guest agent command failed, error was 'Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)'

Shorten this to just just the actual error message.  The guest-exec
example becomes

    Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 24 ++++++++----------------
 qga/commands.c       |  5 ++---
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index ed31077457..0d1b836e87 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -278,8 +278,7 @@ static void acquire_privilege(const char *name, Error **errp)
         TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token))
     {
         if (!LookupPrivilegeValue(NULL, name, &priv.Privileges[0].Luid)) {
-            error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                       "no luid for requested privilege");
+            error_setg(errp, "no luid for requested privilege");
             goto out;
         }
 
@@ -287,14 +286,12 @@ static void acquire_privilege(const char *name, Error **errp)
         priv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
 
         if (!AdjustTokenPrivileges(token, FALSE, &priv, 0, NULL, 0)) {
-            error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                       "unable to acquire requested privilege");
+            error_setg(errp, "unable to acquire requested privilege");
             goto out;
         }
 
     } else {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                   "failed to open privilege token");
+        error_setg(errp, "failed to open privilege token");
     }
 
 out:
@@ -308,8 +305,7 @@ static void execute_async(DWORD WINAPI (*func)(LPVOID), LPVOID opaque,
 {
     HANDLE thread = CreateThread(NULL, 0, func, opaque, 0, NULL);
     if (!thread) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                   "failed to dispatch asynchronous command");
+        error_setg(errp, "failed to dispatch asynchronous command");
     }
 }
 
@@ -1418,22 +1414,19 @@ static void check_suspend_mode(GuestSuspendMode mode, Error **errp)
 
     ZeroMemory(&sys_pwr_caps, sizeof(sys_pwr_caps));
     if (!GetPwrCapabilities(&sys_pwr_caps)) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                   "failed to determine guest suspend capabilities");
+        error_setg(errp, "failed to determine guest suspend capabilities");
         return;
     }
 
     switch (mode) {
     case GUEST_SUSPEND_MODE_DISK:
         if (!sys_pwr_caps.SystemS4) {
-            error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                       "suspend-to-disk not supported by OS");
+            error_setg(errp, "suspend-to-disk not supported by OS");
         }
         break;
     case GUEST_SUSPEND_MODE_RAM:
         if (!sys_pwr_caps.SystemS3) {
-            error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                       "suspend-to-ram not supported by OS");
+            error_setg(errp, "suspend-to-ram not supported by OS");
         }
         break;
     default:
@@ -2175,8 +2168,7 @@ static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp)
     HMODULE module = GetModuleHandle("ntdll");
     PVOID fun = GetProcAddress(module, "RtlGetVersion");
     if (fun == NULL) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED,
-            "Failed to get address of RtlGetVersion");
+        error_setg(errp, "Failed to get address of RtlGetVersion");
         return;
     }
 
diff --git a/qga/commands.c b/qga/commands.c
index 88c1c99fe5..27b16313ea 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -475,7 +475,7 @@ GuestExec *qmp_guest_exec(const char *path,
             guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd : NULL,
             has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
     if (!ret) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
+        error_setg(errp, "%s", gerr->message);
         g_error_free(gerr);
         goto done;
     }
@@ -586,8 +586,7 @@ GuestTimezone *qmp_guest_get_timezone(Error **errp)
     info = g_new0(GuestTimezone, 1);
     tz = g_time_zone_new_local();
     if (tz == NULL) {
-        error_setg(errp, QERR_QGA_COMMAND_FAILED,
-                   "Couldn't retrieve local timezone");
+        error_setg(errp, "Couldn't retrieve local timezone");
         goto error;
     }
 
-- 
2.45.0

Re: [PATCH 2/3] qga: Shorten several error messages

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Some, but not all error messages are of the form
>
>     Guest agent command failed, error was '<actual error message>'
>
> For instance, command guest-exec can fail with an error message like
>
>     Guest agent command failed, error was 'Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)'
>
> Shorten this to just just the actual error message.  The guest-exec
> example becomes
>
>     Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

To be squashed into the patch:

diff --git a/qga/commands.c b/qga/commands.c
index 27b16313ea..5a5fad31f8 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -15,7 +15,6 @@
 #include "guest-agent-core.h"
 #include "qga-qapi-commands.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
 #include "commands-common.h"

Re: [PATCH 2/3] qga: Shorten several error messages

[Philippe Mathieu-Daudé]

On 14/5/24 13:02, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Some, but not all error messages are of the form
>>
>>      Guest agent command failed, error was '<actual error message>'
>>
>> For instance, command guest-exec can fail with an error message like
>>
>>      Guest agent command failed, error was 'Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)'
>>
>> Shorten this to just just the actual error message.  The guest-exec
>> example becomes
>>
>>      Failed to execute child process “/bin/invalid-cmd42” (No such file or directory)
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> [...]
> 
> To be squashed into the patch:
> 
> diff --git a/qga/commands.c b/qga/commands.c
> index 27b16313ea..5a5fad31f8 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -15,7 +15,6 @@
>   #include "guest-agent-core.h"
>   #include "qga-qapi-commands.h"
>   #include "qapi/error.h"
> -#include "qapi/qmp/qerror.h"
>   #include "qemu/base64.h"
>   #include "qemu/cutils.h"
>   #include "commands-common.h"
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/3] qga: Shorten several error messages

[Konstantin Kostiuk]

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

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>



On Tue, May 14, 2024 at 1:58 PM Markus Armbruster <armbru@redhat.com> wrote:

> Some, but not all error messages are of the form
>
>     Guest agent command failed, error was '<actual error message>'
>
> For instance, command guest-exec can fail with an error message like
>
>     Guest agent command failed, error was 'Failed to execute child process
> “/bin/invalid-cmd42�€? (No such file or directory)'
>
> Shorten this to just just the actual error message.  The guest-exec
> example becomes
>
>     Failed to execute child process “/bin/invalid-cmd42�€? (No such file
> or directory)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qga/commands-win32.c | 24 ++++++++----------------
>  qga/commands.c       |  5 ++---
>  2 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index ed31077457..0d1b836e87 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -278,8 +278,7 @@ static void acquire_privilege(const char *name, Error
> **errp)
>          TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token))
>      {
>          if (!LookupPrivilegeValue(NULL, name, &priv.Privileges[0].Luid)) {
> -            error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                       "no luid for requested privilege");
> +            error_setg(errp, "no luid for requested privilege");
>              goto out;
>          }
>
> @@ -287,14 +286,12 @@ static void acquire_privilege(const char *name,
> Error **errp)
>          priv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
>
>          if (!AdjustTokenPrivileges(token, FALSE, &priv, 0, NULL, 0)) {
> -            error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                       "unable to acquire requested privilege");
> +            error_setg(errp, "unable to acquire requested privilege");
>              goto out;
>          }
>
>      } else {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                   "failed to open privilege token");
> +        error_setg(errp, "failed to open privilege token");
>      }
>
>  out:
> @@ -308,8 +305,7 @@ static void execute_async(DWORD WINAPI
> (*func)(LPVOID), LPVOID opaque,
>  {
>      HANDLE thread = CreateThread(NULL, 0, func, opaque, 0, NULL);
>      if (!thread) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                   "failed to dispatch asynchronous command");
> +        error_setg(errp, "failed to dispatch asynchronous command");
>      }
>  }
>
> @@ -1418,22 +1414,19 @@ static void check_suspend_mode(GuestSuspendMode
> mode, Error **errp)
>
>      ZeroMemory(&sys_pwr_caps, sizeof(sys_pwr_caps));
>      if (!GetPwrCapabilities(&sys_pwr_caps)) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                   "failed to determine guest suspend capabilities");
> +        error_setg(errp, "failed to determine guest suspend
> capabilities");
>          return;
>      }
>
>      switch (mode) {
>      case GUEST_SUSPEND_MODE_DISK:
>          if (!sys_pwr_caps.SystemS4) {
> -            error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                       "suspend-to-disk not supported by OS");
> +            error_setg(errp, "suspend-to-disk not supported by OS");
>          }
>          break;
>      case GUEST_SUSPEND_MODE_RAM:
>          if (!sys_pwr_caps.SystemS3) {
> -            error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                       "suspend-to-ram not supported by OS");
> +            error_setg(errp, "suspend-to-ram not supported by OS");
>          }
>          break;
>      default:
> @@ -2175,8 +2168,7 @@ static void ga_get_win_version(RTL_OSVERSIONINFOEXW
> *info, Error **errp)
>      HMODULE module = GetModuleHandle("ntdll");
>      PVOID fun = GetProcAddress(module, "RtlGetVersion");
>      if (fun == NULL) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -            "Failed to get address of RtlGetVersion");
> +        error_setg(errp, "Failed to get address of RtlGetVersion");
>          return;
>      }
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 88c1c99fe5..27b16313ea 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -475,7 +475,7 @@ GuestExec *qmp_guest_exec(const char *path,
>              guest_exec_task_setup, &has_merge, &pid, input_data ? &in_fd
> : NULL,
>              has_output ? &out_fd : NULL, has_output ? &err_fd : NULL,
> &gerr);
>      if (!ret) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> +        error_setg(errp, "%s", gerr->message);
>          g_error_free(gerr);
>          goto done;
>      }
> @@ -586,8 +586,7 @@ GuestTimezone *qmp_guest_get_timezone(Error **errp)
>      info = g_new0(GuestTimezone, 1);
>      tz = g_time_zone_new_local();
>      if (tz == NULL) {
> -        error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -                   "Couldn't retrieve local timezone");
> +        error_setg(errp, "Couldn't retrieve local timezone");
>          goto error;
>      }
>
> --
> 2.45.0
>
>

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

[PATCH 3/3] qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 00b18e9082..390590bb81 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -29,9 +29,6 @@
 #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
     "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")"
 
-#define QERR_QGA_COMMAND_FAILED \
-    "Guest agent command failed, error was '%s'"
-
 #define QERR_UNSUPPORTED \
     "this feature or command is not currently supported"
 
-- 
2.45.0

Re: [PATCH 3/3] qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop

[Philippe Mathieu-Daudé]

On 14/5/24 12:58, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/qmp/qerror.h | 3 ---
>   1 file changed, 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/3] qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop

[Konstantin Kostiuk]

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

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Tue, May 14, 2024 at 1:58 PM Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 00b18e9082..390590bb81 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -29,9 +29,6 @@
>  #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>      "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ",
> maximum: %" PRId64 ")"
>
> -#define QERR_QGA_COMMAND_FAILED \
> -    "Guest agent command failed, error was '%s'"
> -
>  #define QERR_UNSUPPORTED \
>      "this feature or command is not currently supported"
>
> --
> 2.45.0
>
>

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