Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
On 12/6/24 15:07, Markus Armbruster wrote: Philippe Mathieu-Daudé 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-berra...@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 :)
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
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 wrote: > Philippe Mathieu-Daudé 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-berra...@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. > >
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé 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-berra...@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.
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
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é 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é
---
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?
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Please ignore this copy, it has the recipients messed up.
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé 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é
> ---
> 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 li
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé 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é
> ---
> 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 li
[RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
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é
---
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 @@
#
# "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(e
