qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED defini


From: Philippe Mathieu-Daudé
Subject: Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Date: Wed, 12 Jun 2024 14:23:09 +0200
User-agent: Mozilla Thunderbird

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.






reply via email to

[Prev in Thread] Current Thread [Next in Thread]