On Tue, Jun 11, 2024 at 03:55:37PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@redhat.com> writes:
>
> > Rather than creating stubs for every command that just return
> > QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
> > fully exclude generation of the commands on Windows.
> >
> > The command will be rejected at QMP dispatch time instead,
> > avoiding reimplementing rejection by blocking the stub commands.
> >
> > This fixes inconsistency where some commands are implemented
> > as stubs, yet not added to the blockedrpc list.
> >
> > This has the additional benefit that the QGA protocol reference
> > now documents what conditions enable use of the command.
> >
> > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> > ---
> > qga/commands-win32.c | 56 +-------------------------------------------
> > qga/qapi-schema.json | 45 +++++++++++++++++++++++------------
> > 2 files changed, 31 insertions(+), 70 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 9fe670d5b4..2533e4c748 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
>
> [...]
>
> > /* add unsupported commands to the list of blocked RPCs */
> > GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
> > {
> > - const char *list_unsupported[] = {
> > - "guest-suspend-hybrid",
> > - "guest-set-vcpus",
> > - "guest-get-memory-blocks", "guest-set-memory-blocks",
> > - "guest-get-memory-block-info",
> > - NULL};
> > - char **p = (char **)list_unsupported;
> > -
> > - while (*p) {
> > - blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
> > - }
> > -
> > if (!vss_init(true)) {
> > g_debug("vss_init failed, vss commands are going to be disabled");
> > const char *list[] = {
> > "guest-get-fsinfo", "guest-fsfreeze-status",
> > "guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL};
> > - p = (char **)list;
> > + char **p = (char **)list;
> >
> > while (*p) {
> > blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
> }
> }
>
> return blockedrpcs;
> }
>
> Four commands get disabled when vss_init() fails, i.e. when qga-vss.dll
> can't be loaded and initialized.
>
> Three of the four commands do this first:
>
> if (!vss_initialized()) {
> error_setg(errp, QERR_UNSUPPORTED);
> return 0;
> }
>
> The execption is qmp_guest_get_fsinfo().
>
> vss_initialized() returns true between successful vss_init() and
> vss_deinit().
>
> Aside: we call vss_init() in three places. Two of them init, call
> something, then deinit. Weird. Moving on.
>
> If these commands are meant to be only available when the DLL is, then
> having them check vss_initialized() is useless.
>
> Conversely, if the check isn't useless, then the "make it available
> only" business is.
>
> Opportunity for further cleanup?
If we eliminate the "make it available" check in ga_command_init_blockedrpcs,
that would be a nice cleanup IMHO, as these few commands are the only
special case where that's needed now.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
