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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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