diff mbox series

[RFC,v2,21/22] qapi: Inline and remove QERR_UNSUPPORTED definition

Message ID 20231005045041.52649-22-philmd@linaro.org
State New
Headers show
Series qapi: Kill 'qapi/qmp/qerror.h' for good | expand

Commit Message

Philippe Mathieu-Daudé Oct. 5, 2023, 4:50 a.m. UTC
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:

  /*
   * These macros will go away, please don't use
   * in new code, and do not add new ones!
   */

Mechanical transformation using:

  $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently supported"/' \
    $(git grep -wl QERR_UNSUPPORTED)

then manually removing the definition in include/qapi/qmp/qerror.h.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Not sure what is the best way to change the comment
     in qga/qapi-schema.json...
---
 qga/qapi-schema.json      |  5 +++--
 include/qapi/qmp/qerror.h |  3 ---
 qga/commands-bsd.c        |  8 +++----
 qga/commands-posix.c      | 46 +++++++++++++++++++--------------------
 qga/commands-win32.c      | 22 +++++++++----------
 5 files changed, 41 insertions(+), 43 deletions(-)

Comments

Markus Armbruster Oct. 5, 2023, 11:22 a.m. UTC | #1
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
>   /*
>    * These macros will go away, please don't use
>    * in new code, and do not add new ones!
>    */
>
> Mechanical transformation using:
>
>   $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently supported"/' \
>     $(git grep -wl QERR_UNSUPPORTED)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Not sure what is the best way to change the comment
>      in qga/qapi-schema.json...
> ---
>  qga/qapi-schema.json      |  5 +++--
>  include/qapi/qmp/qerror.h |  3 ---
>  qga/commands-bsd.c        |  8 +++----
>  qga/commands-posix.c      | 46 +++++++++++++++++++--------------------
>  qga/commands-win32.c      | 22 +++++++++----------
>  5 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b720dd4379..51683f4dc2 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -6,8 +6,9 @@
   ##
   # = General note concerning the use of guest agent interfaces
>  #
>  # "unsupported" is a higher-level error than the errors that
>  # individual commands might document.  The caller should always be
> -# prepared to receive QERR_UNSUPPORTED, even if the given command
> -# doesn't specify it, or doesn't document any failure mode at all.
> +# prepared to receive the "this feature or command is not currently supported"
> +# message, even if the given command doesn't specify it, or doesn't document
> +# any failure mode at all.
>  ##
>  
>  ##

The comment is problematic before the patch.  It's a doc comment,
i.e. it goes into the "QEMU Guest Agent Protocol Reference" manual,
where QERR_UNSUPPORTED is meaningless.  Back when the comment was added
(commit 9481ecd737b "qga schema: document generic QERR_UNSUPPORTED"), it
was still internal documentation, where QERR_UNSUPPORTED made sense.  It
became external documentation four years later (commit 56e8bdd46a8
"build-sys: add qapi doc generation targets")

I'm not sure how useful the comment is.

I guess we could instead simply point out that clients should always be
prepared for errors, even when the command doesn't document any, simply
because commands need not exist in all versions or builds of qemu-ga.

> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 840831cc6a..7606f4525d 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,7 +17,4 @@
>   * add new ones!
>   */
>  
> -#define QERR_UNSUPPORTED \
> -    "this feature or command is not currently supported"
> -
>  #endif /* QERROR_H */
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index 17bddda1cf..11536f148f 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -152,25 +152,25 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
>  
>  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

These are all commands that are not supported by this build of qemu-ga.
We could use the opportunity to improve the error message: scratch
"feature or ".  Or maybe change the message to "this command is not
supported in this version of qemu-ga".

More of the same below, marked [*].

Taking a step back...  Do we really need to make this a failure of its
own?  Why not fail exactly as if the command didn't exist?  Why would a
client ever care for the difference between "command doesn't exist in
this build of qemu-ga (but it does in other builds of this version of
qemu-ga)" and "command doesn't exist in this build of qemu-ga (and it
won't in any build of this version of qemu-ga)"?

If clients don't care, we could instead unregister these commands,
i.e. undo qmp_register_command().  The command will then fail exactly
like any other unknown command.  We still need to provide the functions
to make the linker happy (unless we play with weak symbols), but they
can simply abort().

Michael and/or Konstantin, do you have comments as maintainers?  A
preference even?

Hmm, there's yet another mechanism to disable commands:
qmp_disable_command() & friends.  Looks like its purpose is letting
guest system administrators disable commands, and also to disable a
bunch of commands in "frozen state", whatever that is.  Alright, I
didn't see that, lalala.

>  #endif /* CONFIG_FSFREEZE */
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6169bbf7a0..f510add366 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -165,7 +165,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>      }
>  
>      if (!hwclock_available) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return;
>      }
>  

!hwclock_available means /sbin/hwclock grants execute permission for the
process's real UID and GID.  I'm not sure why we bother to check that.
Why not let execl() fail, and report that error?  Oh, we don't:

           /* 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);

This is wrong.  We should error_setg_errno(errp, errno, ...); return
instead.  Not your patch's problem.

Until we do that: [*]

> @@ -1540,7 +1540,7 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

Command not supported by this build of qemu-qa: [*].

>  
> @@ -2235,7 +2235,7 @@ void qmp_guest_set_user_password(const char *username,
>                                   bool crypted,
>                                   Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  #endif /* __linux__ || __FreeBSD__ */
>  
> @@ -2751,47 +2751,47 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  
>  void qmp_guest_suspend_disk(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  void qmp_guest_suspend_ram(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  void qmp_guest_suspend_hybrid(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return -1;
>  }
>  
>  GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockResponseList *
>  qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3056,7 +3056,7 @@ error:
>  
>  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3066,20 +3066,20 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  
>  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  int64_t qmp_guest_fsfreeze_freeze(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
> @@ -3088,33 +3088,33 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>                                         strList *mountpoints,
>                                         Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3124,7 +3124,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  GuestFilesystemTrimResponse *
>  qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  #endif
> @@ -3243,7 +3243,7 @@ GuestUserList *qmp_guest_get_users(Error **errp)
>  
>  GuestUserList *qmp_guest_get_users(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3386,7 +3386,7 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>  GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return NULL;
>  }

Commands not supported by this build of qemu-qa: [*].

> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index aa8c9770d4..5c9f8e0923 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1213,7 +1213,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>  {
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  
> @@ -1241,7 +1241,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>      Error *local_err = NULL;
>  
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  
> @@ -1276,7 +1276,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>      int i;
>  
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  

!vss_initialized() means qga-vss.dll failed to load and initialize.

Another [*].

> @@ -1509,7 +1509,7 @@ out:
>  
>  void qmp_guest_suspend_hybrid(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
> @@ -1877,7 +1877,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  
>  int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return -1;
>  }
>  

Commands not supported by this build of qemu-qa: [*].

> @@ -1938,7 +1938,7 @@ void qmp_guest_set_user_password(const char *username,
>      GError *gerr = NULL;
>  
>      if (crypted) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return;
>      }

Command does not support "crypted": true in this build of qemu-ga.  The
error message is crap.  Better would be some variation of "this machine
supports only clear-text passwords".

>  
> @@ -1983,20 +1983,20 @@ done:
>  
>  GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockResponseList *
>  qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -2522,12 +2522,12 @@ char *qga_get_host_name(Error **errp)
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

Commands not supported by this build of qemu-qa: [*].

Summary of my review:

* You're unsure about your change to the "General note concerning the
  use of guest agent interfaces".  I suggested a way to rewrite it.

* Error messages could use improvement.  Since your patch doesn't change
  any, feel free to leave that for another day.

* How we do compiled-out commands could use improvement.  Feel even more
  free to leave for another day.  I'd like to hear the maintainers'
  opinion, though.
Markus Armbruster Oct. 5, 2023, 11:57 a.m. UTC | #2
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
>   /*
>    * These macros will go away, please don't use
>    * in new code, and do not add new ones!
>    */
>
> Mechanical transformation using:
>
>   $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently supported"/' \
>     $(git grep -wl QERR_UNSUPPORTED)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Not sure what is the best way to change the comment
>      in qga/qapi-schema.json...
> ---
>  qga/qapi-schema.json      |  5 +++--
>  include/qapi/qmp/qerror.h |  3 ---
>  qga/commands-bsd.c        |  8 +++----
>  qga/commands-posix.c      | 46 +++++++++++++++++++--------------------
>  qga/commands-win32.c      | 22 +++++++++----------
>  5 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b720dd4379..51683f4dc2 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -6,8 +6,9 @@
   ##
   # = General note concerning the use of guest agent interfaces
>  #
>  # "unsupported" is a higher-level error than the errors that
>  # individual commands might document.  The caller should always be
> -# prepared to receive QERR_UNSUPPORTED, even if the given command
> -# doesn't specify it, or doesn't document any failure mode at all.
> +# prepared to receive the "this feature or command is not currently supported"
> +# message, even if the given command doesn't specify it, or doesn't document
> +# any failure mode at all.
>  ##
>  
>  ##

The comment is problematic before the patch.  It's a doc comment,
i.e. it goes into the "QEMU Guest Agent Protocol Reference" manual,
where QERR_UNSUPPORTED is meaningless.  Back when the comment was added
(commit 9481ecd737b "qga schema: document generic QERR_UNSUPPORTED"), it
was still internal documentation, where QERR_UNSUPPORTED made sense.  It
became external documentation four years later (commit 56e8bdd46a8
"build-sys: add qapi doc generation targets")

I'm not sure how useful the comment is.

I guess we could instead simply point out that clients should always be
prepared for errors, even when the command doesn't document any, simply
because commands need not exist in all versions or builds of qemu-ga.

> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 840831cc6a..7606f4525d 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,7 +17,4 @@
>   * add new ones!
>   */
>  
> -#define QERR_UNSUPPORTED \
> -    "this feature or command is not currently supported"
> -
>  #endif /* QERROR_H */
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index 17bddda1cf..11536f148f 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -152,25 +152,25 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
>  
>  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

These are all commands that are not supported by this build of qemu-ga.
We could use the opportunity to improve the error message: scratch
"feature or ".  Or maybe change the message to "this command is not
supported in this version of qemu-ga".

More of the same below, marked [*].

Taking a step back...  Do we really need to make this a failure of its
own?  Why not fail exactly as if the command didn't exist?  Why would a
client ever care for the difference between "command doesn't exist in
this build of qemu-ga (but it does in other builds of this version of
qemu-ga)" and "command doesn't exist in this build of qemu-ga (and it
won't in any build of this version of qemu-ga)"?

If clients don't care, we could instead unregister these commands,
i.e. undo qmp_register_command().  The command will then fail exactly
like any other unknown command.  We still need to provide the functions
to make the linker happy (unless we play with weak symbols), but they
can simply abort().

Michael and/or Konstantin, do you have comments as maintainers?  A
preference even?

Hmm, there's yet another mechanism to disable commands:
qmp_disable_command() & friends.  Looks like its purpose is letting
guest system administrators disable commands, and also to disable a
bunch of commands in "frozen state", whatever that is.  Alright, I
didn't see that, lalala.

>  #endif /* CONFIG_FSFREEZE */
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6169bbf7a0..f510add366 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -165,7 +165,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>      }
>  
>      if (!hwclock_available) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return;
>      }
>  

!hwclock_available means /sbin/hwclock grants execute permission for the
process's real UID and GID.  I'm not sure why we bother to check that.
Why not let execl() fail, and report that error?  Oh, we don't:

           /* 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);

This is wrong.  We should error_setg_errno(errp, errno, ...); return
instead.  Not your patch's problem.

Until we do that: [*]

> @@ -1540,7 +1540,7 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

Command not supported by this build of qemu-qa: [*].

>  
> @@ -2235,7 +2235,7 @@ void qmp_guest_set_user_password(const char *username,
>                                   bool crypted,
>                                   Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  #endif /* __linux__ || __FreeBSD__ */
>  
> @@ -2751,47 +2751,47 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  
>  void qmp_guest_suspend_disk(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  void qmp_guest_suspend_ram(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  void qmp_guest_suspend_hybrid(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return -1;
>  }
>  
>  GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockResponseList *
>  qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3056,7 +3056,7 @@ error:
>  
>  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3066,20 +3066,20 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  
>  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  int64_t qmp_guest_fsfreeze_freeze(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
> @@ -3088,33 +3088,33 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>                                         strList *mountpoints,
>                                         Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return 0;
>  }
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3124,7 +3124,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  GuestFilesystemTrimResponse *
>  qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  #endif
> @@ -3243,7 +3243,7 @@ GuestUserList *qmp_guest_get_users(Error **errp)
>  
>  GuestUserList *qmp_guest_get_users(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -3386,7 +3386,7 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>  GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  
>      return NULL;
>  }

Commands not supported by this build of qemu-qa: [*].

> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index aa8c9770d4..5c9f8e0923 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1213,7 +1213,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>  GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>  {
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  
> @@ -1241,7 +1241,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>      Error *local_err = NULL;
>  
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  
> @@ -1276,7 +1276,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>      int i;
>  
>      if (!vss_initialized()) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return 0;
>      }
>  

!vss_initialized() means qga-vss.dll failed to load and initialize.

Another [*].

> @@ -1509,7 +1509,7 @@ out:
>  
>  void qmp_guest_suspend_hybrid(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>  }
>  
>  static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
> @@ -1877,7 +1877,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  
>  int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return -1;
>  }
>  

Commands not supported by this build of qemu-qa: [*].

> @@ -1938,7 +1938,7 @@ void qmp_guest_set_user_password(const char *username,
>      GError *gerr = NULL;
>  
>      if (crypted) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp, "this feature or command is not currently supported");
>          return;
>      }

Command does not support "crypted": true in this build of qemu-ga.  The
error message is crap.  Better would be some variation of "this machine
supports only clear-text passwords".

>  
> @@ -1983,20 +1983,20 @@ done:
>  
>  GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockResponseList *
>  qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
> @@ -2522,12 +2522,12 @@ char *qga_get_host_name(Error **errp)
>  
>  GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }
>  
>  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  {
> -    error_setg(errp, QERR_UNSUPPORTED);
> +    error_setg(errp, "this feature or command is not currently supported");
>      return NULL;
>  }

Commands not supported by this build of qemu-qa: [*].

Summary of my review:

* You're unsure about your change to the "General note concerning the
  use of guest agent interfaces".  I suggested a way to rewrite it.

* Error messages could use improvement.  Since your patch doesn't change
  any, feel free to leave that for another day.

* How we do compiled-out commands could use improvement.  Feel even more
  free to leave for another day.  I'd like to hear the maintainers'
  opinion, though.
Markus Armbruster Oct. 5, 2023, 11:57 a.m. UTC | #3
Please ignore this copy, it has the recipients messed up.
Philippe Mathieu-Daudé June 12, 2024, 12:23 p.m. UTC | #4
Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA.

Do you mind giving our maintainer's position on Markus
analysis so we can know how to proceed with this definition?

Regards,

Phil.

On 5/10/23 13:22, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Address the comment added in commit 4629ed1e98
>> ("qerror: Finally unused, clean up"), from 2015:
>>
>>    /*
>>     * These macros will go away, please don't use
>>     * in new code, and do not add new ones!
>>     */
>>
>> Mechanical transformation using:
>>
>>    $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently supported"/' \
>>      $(git grep -wl QERR_UNSUPPORTED)
>>
>> then manually removing the definition in include/qapi/qmp/qerror.h.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC: Not sure what is the best way to change the comment
>>       in qga/qapi-schema.json...
>> ---
>>   qga/qapi-schema.json      |  5 +++--
>>   include/qapi/qmp/qerror.h |  3 ---
>>   qga/commands-bsd.c        |  8 +++----
>>   qga/commands-posix.c      | 46 +++++++++++++++++++--------------------
>>   qga/commands-win32.c      | 22 +++++++++----------
>>   5 files changed, 41 insertions(+), 43 deletions(-)
>>
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index b720dd4379..51683f4dc2 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -6,8 +6,9 @@
>     ##
>     # = General note concerning the use of guest agent interfaces
>>   #
>>   # "unsupported" is a higher-level error than the errors that
>>   # individual commands might document.  The caller should always be
>> -# prepared to receive QERR_UNSUPPORTED, even if the given command
>> -# doesn't specify it, or doesn't document any failure mode at all.
>> +# prepared to receive the "this feature or command is not currently supported"
>> +# message, even if the given command doesn't specify it, or doesn't document
>> +# any failure mode at all.
>>   ##
>>   
>>   ##
> 
> The comment is problematic before the patch.  It's a doc comment,
> i.e. it goes into the "QEMU Guest Agent Protocol Reference" manual,
> where QERR_UNSUPPORTED is meaningless.  Back when the comment was added
> (commit 9481ecd737b "qga schema: document generic QERR_UNSUPPORTED"), it
> was still internal documentation, where QERR_UNSUPPORTED made sense.  It
> became external documentation four years later (commit 56e8bdd46a8
> "build-sys: add qapi doc generation targets")
> 
> I'm not sure how useful the comment is.
> 
> I guess we could instead simply point out that clients should always be
> prepared for errors, even when the command doesn't document any, simply
> because commands need not exist in all versions or builds of qemu-ga.
> 
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 840831cc6a..7606f4525d 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -17,7 +17,4 @@
>>    * add new ones!
>>    */
>>   
>> -#define QERR_UNSUPPORTED \
>> -    "this feature or command is not currently supported"
>> -
>>   #endif /* QERROR_H */
>> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
>> index 17bddda1cf..11536f148f 100644
>> --- a/qga/commands-bsd.c
>> +++ b/qga/commands-bsd.c
>> @@ -152,25 +152,25 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
>>   
>>   GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
> 
> These are all commands that are not supported by this build of qemu-ga.
> We could use the opportunity to improve the error message: scratch
> "feature or ".  Or maybe change the message to "this command is not
> supported in this version of qemu-ga".
> 
> More of the same below, marked [*].
> 
> Taking a step back...  Do we really need to make this a failure of its
> own?  Why not fail exactly as if the command didn't exist?  Why would a
> client ever care for the difference between "command doesn't exist in
> this build of qemu-ga (but it does in other builds of this version of
> qemu-ga)" and "command doesn't exist in this build of qemu-ga (and it
> won't in any build of this version of qemu-ga)"?
> 
> If clients don't care, we could instead unregister these commands,
> i.e. undo qmp_register_command().  The command will then fail exactly
> like any other unknown command.  We still need to provide the functions
> to make the linker happy (unless we play with weak symbols), but they
> can simply abort().
> 
> Michael and/or Konstantin, do you have comments as maintainers?  A
> preference even?
> 
> Hmm, there's yet another mechanism to disable commands:
> qmp_disable_command() & friends.  Looks like its purpose is letting
> guest system administrators disable commands, and also to disable a
> bunch of commands in "frozen state", whatever that is.  Alright, I
> didn't see that, lalala.
> 
>>   #endif /* CONFIG_FSFREEZE */
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 6169bbf7a0..f510add366 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -165,7 +165,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>>       }
>>   
>>       if (!hwclock_available) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp, "this feature or command is not currently supported");
>>           return;
>>       }
>>   
> 
> !hwclock_available means /sbin/hwclock grants execute permission for the
> process's real UID and GID.  I'm not sure why we bother to check that.
> Why not let execl() fail, and report that error?  Oh, we don't:
> 
>             /* 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);
> 
> This is wrong.  We should error_setg_errno(errp, errno, ...); return
> instead.  Not your patch's problem.
> 
> Until we do that: [*]
> 
>> @@ -1540,7 +1540,7 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>>   
>>   GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
> 
> Command not supported by this build of qemu-qa: [*].
> 
>>   
>> @@ -2235,7 +2235,7 @@ void qmp_guest_set_user_password(const char *username,
>>                                    bool crypted,
>>                                    Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   }
>>   #endif /* __linux__ || __FreeBSD__ */
>>   
>> @@ -2751,47 +2751,47 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>>   
>>   void qmp_guest_suspend_disk(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   }
>>   
>>   void qmp_guest_suspend_ram(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   }
>>   
>>   void qmp_guest_suspend_hybrid(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   }
>>   
>>   GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return -1;
>>   }
>>   
>>   GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestMemoryBlockResponseList *
>>   qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>> @@ -3056,7 +3056,7 @@ error:
>>   
>>   GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>> @@ -3066,20 +3066,20 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>>   
>>   GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   
>>       return 0;
>>   }
>>   
>>   int64_t qmp_guest_fsfreeze_freeze(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   
>>       return 0;
>>   }
>> @@ -3088,33 +3088,33 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>>                                          strList *mountpoints,
>>                                          Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   
>>       return 0;
>>   }
>>   
>>   int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   
>>       return 0;
>>   }
>>   
>>   GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>> @@ -3124,7 +3124,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>>   GuestFilesystemTrimResponse *
>>   qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   #endif
>> @@ -3243,7 +3243,7 @@ GuestUserList *qmp_guest_get_users(Error **errp)
>>   
>>   GuestUserList *qmp_guest_get_users(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>> @@ -3386,7 +3386,7 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>>   
>>   GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   
>>       return NULL;
>>   }
> 
> Commands not supported by this build of qemu-qa: [*].
> 
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index aa8c9770d4..5c9f8e0923 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -1213,7 +1213,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>>   GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>>   {
>>       if (!vss_initialized()) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp, "this feature or command is not currently supported");
>>           return 0;
>>       }
>>   
>> @@ -1241,7 +1241,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>>       Error *local_err = NULL;
>>   
>>       if (!vss_initialized()) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp, "this feature or command is not currently supported");
>>           return 0;
>>       }
>>   
>> @@ -1276,7 +1276,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>>       int i;
>>   
>>       if (!vss_initialized()) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp, "this feature or command is not currently supported");
>>           return 0;
>>       }
>>   
> 
> !vss_initialized() means qga-vss.dll failed to load and initialize.
> 
> Another [*].
> 
>> @@ -1509,7 +1509,7 @@ out:
>>   
>>   void qmp_guest_suspend_hybrid(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>   }
>>   
>>   static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
>> @@ -1877,7 +1877,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>   
>>   int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return -1;
>>   }
>>   
> 
> Commands not supported by this build of qemu-qa: [*].
> 
>> @@ -1938,7 +1938,7 @@ void qmp_guest_set_user_password(const char *username,
>>       GError *gerr = NULL;
>>   
>>       if (crypted) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp, "this feature or command is not currently supported");
>>           return;
>>       }
> 
> Command does not support "crypted": true in this build of qemu-ga.  The
> error message is crap.  Better would be some variation of "this machine
> supports only clear-text passwords".
> 
>>   
>> @@ -1983,20 +1983,20 @@ done:
>>   
>>   GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestMemoryBlockResponseList *
>>   qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>> @@ -2522,12 +2522,12 @@ char *qga_get_host_name(Error **errp)
>>   
>>   GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
>>   
>>   GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>>   {
>> -    error_setg(errp, QERR_UNSUPPORTED);
>> +    error_setg(errp, "this feature or command is not currently supported");
>>       return NULL;
>>   }
> 
> Commands not supported by this build of qemu-qa: [*].
> 
> Summary of my review:
> 
> * You're unsure about your change to the "General note concerning the
>    use of guest agent interfaces".  I suggested a way to rewrite it.
> 
> * Error messages could use improvement.  Since your patch doesn't change
>    any, feel free to leave that for another day.
> 
> * How we do compiled-out commands could use improvement.  Feel even more
>    free to leave for another day.  I'd like to hear the maintainers'
>    opinion, though.
> 
>
Markus Armbruster June 12, 2024, 1:07 p.m. UTC | #5
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA.
>
> Do you mind giving our maintainer's position on Markus
> analysis so we can know how to proceed with this definition?

Daniel Berrangé recently posted patches that get rid of most instances
of QERR_UNSUPPORTED:

    [PATCH 00/20] qga: clean up command source locations and conditionals
    https://lore.kernel.org/qemu-devel/20240604134933.220112-1-berrange@redhat.com/

I pointed out a possible opportunity to remove even more.

I think we should let the dust settle there, then figure out how to
eliminate remaining QERR_UNSUPPORTED, if any.
Konstantin Kostiuk June 12, 2024, 1:27 p.m. UTC | #6
Hi Markus and Philippe,

I agree to remove QERR_UNSUPPORTED and have more specific errors
or even remove the functions from the schema in some cases instead of
copy-paste QERR_UNSUPPORTED from platform to platform.

Best Regards,
Konstantin Kostiuk.


On Wed, Jun 12, 2024 at 4:07 PM Markus Armbruster <armbru@redhat.com> wrote:

> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA.
> >
> > Do you mind giving our maintainer's position on Markus
> > analysis so we can know how to proceed with this definition?
>
> Daniel Berrangé recently posted patches that get rid of most instances
> of QERR_UNSUPPORTED:
>
>     [PATCH 00/20] qga: clean up command source locations and conditionals
>
> https://lore.kernel.org/qemu-devel/20240604134933.220112-1-berrange@redhat.com/
>
> I pointed out a possible opportunity to remove even more.
>
> I think we should let the dust settle there, then figure out how to
> eliminate remaining QERR_UNSUPPORTED, if any.
>
>
Philippe Mathieu-Daudé June 12, 2024, 1:45 p.m. UTC | #7
On 12/6/24 15:07, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA.
>>
>> Do you mind giving our maintainer's position on Markus
>> analysis so we can know how to proceed with this definition?
> 
> Daniel Berrangé recently posted patches that get rid of most instances
> of QERR_UNSUPPORTED:
> 
>      [PATCH 00/20] qga: clean up command source locations and conditionals
>      https://lore.kernel.org/qemu-devel/20240604134933.220112-1-berrange@redhat.com/
> 
> I pointed out a possible opportunity to remove even more.
> 
> I think we should let the dust settle there, then figure out how to
> eliminate remaining QERR_UNSUPPORTED, if any.

Ah great, thanks for the update :)
diff mbox series

Patch

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b720dd4379..51683f4dc2 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -6,8 +6,9 @@ 
 #
 # "unsupported" is a higher-level error than the errors that
 # individual commands might document.  The caller should always be
-# prepared to receive QERR_UNSUPPORTED, even if the given command
-# doesn't specify it, or doesn't document any failure mode at all.
+# prepared to receive the "this feature or command is not currently supported"
+# message, even if the given command doesn't specify it, or doesn't document
+# any failure mode at all.
 ##
 
 ##
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 840831cc6a..7606f4525d 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,7 +17,4 @@ 
  * add new ones!
  */
 
-#define QERR_UNSUPPORTED \
-    "this feature or command is not currently supported"
-
 #endif /* QERROR_H */
diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
index 17bddda1cf..11536f148f 100644
--- a/qga/commands-bsd.c
+++ b/qga/commands-bsd.c
@@ -152,25 +152,25 @@  int qmp_guest_fsfreeze_do_thaw(Error **errp)
 
 GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 #endif /* CONFIG_FSFREEZE */
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6169bbf7a0..f510add366 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -165,7 +165,7 @@  void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     }
 
     if (!hwclock_available) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "this feature or command is not currently supported");
         return;
     }
 
@@ -1540,7 +1540,7 @@  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
 
 GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -2235,7 +2235,7 @@  void qmp_guest_set_user_password(const char *username,
                                  bool crypted,
                                  Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 }
 #endif /* __linux__ || __FreeBSD__ */
 
@@ -2751,47 +2751,47 @@  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 
 void qmp_guest_suspend_disk(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 }
 
 void qmp_guest_suspend_ram(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 }
 
 void qmp_guest_suspend_hybrid(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 }
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return -1;
 }
 
 GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestMemoryBlockResponseList *
 qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -3056,7 +3056,7 @@  error:
 
 GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -3066,20 +3066,20 @@  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 
 GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 
     return 0;
 }
 
 int64_t qmp_guest_fsfreeze_freeze(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 
     return 0;
 }
@@ -3088,33 +3088,33 @@  int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
                                        strList *mountpoints,
                                        Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 
     return 0;
 }
 
 int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 
     return 0;
 }
 
 GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -3124,7 +3124,7 @@  GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 GuestFilesystemTrimResponse *
 qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 #endif
@@ -3243,7 +3243,7 @@  GuestUserList *qmp_guest_get_users(Error **errp)
 
 GuestUserList *qmp_guest_get_users(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -3386,7 +3386,7 @@  GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
 GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 
     return NULL;
 }
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index aa8c9770d4..5c9f8e0923 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1213,7 +1213,7 @@  GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
 GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
 {
     if (!vss_initialized()) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "this feature or command is not currently supported");
         return 0;
     }
 
@@ -1241,7 +1241,7 @@  int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
     Error *local_err = NULL;
 
     if (!vss_initialized()) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "this feature or command is not currently supported");
         return 0;
     }
 
@@ -1276,7 +1276,7 @@  int64_t qmp_guest_fsfreeze_thaw(Error **errp)
     int i;
 
     if (!vss_initialized()) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "this feature or command is not currently supported");
         return 0;
     }
 
@@ -1509,7 +1509,7 @@  out:
 
 void qmp_guest_suspend_hybrid(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
 }
 
 static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
@@ -1877,7 +1877,7 @@  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 
 int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return -1;
 }
 
@@ -1938,7 +1938,7 @@  void qmp_guest_set_user_password(const char *username,
     GError *gerr = NULL;
 
     if (crypted) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "this feature or command is not currently supported");
         return;
     }
 
@@ -1983,20 +1983,20 @@  done:
 
 GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestMemoryBlockResponseList *
 qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
@@ -2522,12 +2522,12 @@  char *qga_get_host_name(Error **errp)
 
 GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }
 
 GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    error_setg(errp, "this feature or command is not currently supported");
     return NULL;
 }