| Message ID | 20231005045041.52649-22-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Kill 'qapi/qmp/qerror.h' for good | expand |
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.
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.
Please ignore this copy, it has the recipients messed up.
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. > >
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.
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. > >
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 --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; }
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(-)