On Thu, Jun 13, 2024 at 2:43 PM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> 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.
>
> The two odd balls are a special case for the Windows only --service
> argument, which installs a Windows system service for VSS. In that
> case, the QGA immediately exits after (un)installing the service.
> So the vss_init+vss_deinit pairly makes sense there.
>
> > If these commands are meant to be only available when the DLL is, then
> > having them check vss_initialized() is useless.
>
> The DLL should always exist unless the install is broken, but versions
> of Windows prior to win2k3 don't support the required APIs, so vss_init
> could fail.
>
If Windows VSS is disabled vss_init could fail too. Also, one person
reported
an error that VSS failed to initialize after the first boot. I can't
reproduce this
so I have not details but we should check in runtime that QGA-VSS is
initialized
before trying to freeze fs, in case to be more sure where is error.
>
> > Conversely, if the check isn't useless, then the "make it available
> > only" business is.
>
> The 'make it available only" business is bad practice, as it does not
> allow a caller to distinguish between the admin manually disabling
> these commands, vs the commands being unavailable due to the OS not
> supporting the feature. This distinction is important to preserve
> to benefit those triaging bugs about why this might fail.
>
> I'm going to get rid of the runtime blocking in
> ga_command_init_blockedrpcs,
> and also replace QERR_UNSUPPORTED with a more targetted error message to
> clearly articulate why the commands are failing.
>
> 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 :|
>
>
