Patchwork [v4,08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler

login
register
mail settings
Submitter Tomoki Sekiyama
Date June 6, 2013, 3:06 p.m.
Message ID <20130606150653.10486.34913.stgit@hds.com>
Download mbox | patch
Permalink /patch/249452/
State New
Headers show

Comments

Tomoki Sekiyama - June 6, 2013, 3:06 p.m.
Support guest-fsfreeze-freeze and guest-fsfreeze-thaw commands for Windows
guests. When fsfreeze command is issued, it calls the VSS requester to
freeze filesystems and applications. On thaw command, it again tells the VSS
requester to thaw them.

This also adds calling of initialize functions for the VSS requester.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/commands-win32.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++----
 qga/main.c           |   33 ++++++++++++++++++++++
 2 files changed, 100 insertions(+), 7 deletions(-)
Laszlo Ersek - July 1, 2013, 1:29 p.m.
some comments below

On 06/06/13 17:06, Tomoki Sekiyama wrote:
> Support guest-fsfreeze-freeze and guest-fsfreeze-thaw commands for Windows
> guests. When fsfreeze command is issued, it calls the VSS requester to
> freeze filesystems and applications. On thaw command, it again tells the VSS
> requester to thaw them.
> 
> This also adds calling of initialize functions for the VSS requester.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
>  qga/commands-win32.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++----
>  qga/main.c           |   33 ++++++++++++++++++++++
>  2 files changed, 100 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 24e4ad0..67dca60 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -15,6 +15,7 @@
>  #include <wtypes.h>
>  #include <powrprof.h>
>  #include "qga/guest-agent-core.h"
> +#include "qga/vss-win32-requester.h"
>  #include "qga-qmp-commands.h"
>  #include "qapi/qmp/qerror.h"
>  
> @@ -151,34 +152,95 @@ void qmp_guest_file_flush(int64_t handle, Error **err)
>      error_set(err, QERR_UNSUPPORTED);
>  }
>  
> +#ifdef HAS_VSS_SDK
> +

(CONFIG_... as we've discussed)

>  /*
>   * Return status of freeze/thaw
>   */
>  GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>  {
> -    error_set(err, QERR_UNSUPPORTED);
> -    return 0;
> +    if (!vss_initialized()) {
> +        error_set(err, QERR_UNSUPPORTED);
> +        return 0;
> +    }
> +
> +    if (ga_is_frozen(ga_state)) {
> +        return GUEST_FSFREEZE_STATUS_FROZEN;
> +    }
> +
> +    return GUEST_FSFREEZE_STATUS_THAWED;
>  }
>  
>  /*
> - * Walk list of mounted file systems in the guest, and freeze the ones which
> - * are real local file systems.
> + * Freeze local file systems using Volume Shadow-copy Service.
> + * The frozen state is limited for up to 10 seconds by VSS.
>   */
>  int64_t qmp_guest_fsfreeze_freeze(Error **err)
>  {
> -    error_set(err, QERR_UNSUPPORTED);
> +    int i;
> +
> +    slog("guest-fsfreeze called");
> +
> +    if (!vss_initialized()) {
> +        error_set(err, QERR_UNSUPPORTED);
> +        return 0;
> +    }
> +
> +    /* cannot risk guest agent blocking itself on a write in this state */
> +    ga_set_frozen(ga_state);

Great! I wasn't expecting this, but in retrospect I don't know why :)

> +
> +    qga_vss_fsfreeze_freeze(&i, err);
> +    if (error_is_set(err)) {
> +        goto error;
> +    }
> +
> +    return i;
> +
> +error:
> +    qmp_guest_fsfreeze_thaw(NULL);

Passing NULL here as "errp" concerns me slightly. I've been assuming
that "errp" is never NULL due to the outermost QMP layer always wanting
to receive errors and to serialize them.

Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
questions would answer with false. That said, nothing seems to be
affected right now.

Maybe a dummy local variable would be more future-proof... OTOH it would
be stylistically questionable :)


Because of the initial hEvent2 check in qga_vss_fsfreeze_thaw(), it
should be safe to call at any time AFAICS. OK.

>      return 0;
>  }
>  
>  /*
> - * Walk list of frozen file systems in the guest, and thaw them.
> + * Thaw local file systems using Volume Shadow-copy Service.
>   */
>  int64_t qmp_guest_fsfreeze_thaw(Error **err)
>  {
> +    int i;
> +
> +    if (!vss_initialized()) {
> +        error_set(err, QERR_UNSUPPORTED);
> +        return 0;
> +    }
> +
> +    qga_vss_fsfreeze_thaw(&i, err);

I wonder how libvirt interprets a failure to thaw -- does it expect
filesystems to remain frozen? (CC'ing Eric.)

For example, the VSS_E_HOLD_WRITES_TIMEOUT error is reported on the next
thaw attempt, but when this error occurs, filesystems have been
auto-thawed I think.

Anyway I can't suggest anything fail-safe here.

> +
> +    ga_unset_frozen(ga_state);
> +    return i;
> +}
> +
> +#else

(maybe mention the macro in a comment that the #else depends on here)

> +
> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> +{
> +    error_set(err, QERR_UNSUPPORTED);
> +    return 0;
> +}
> +
> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> +{
> +    error_set(err, QERR_UNSUPPORTED);
> +    return 0;
> +}
> +
> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> +{
>      error_set(err, QERR_UNSUPPORTED);
>      return 0;
>  }
>  
> +#endif
> +
>  /*
>   * Walk list of mounted file systems in the guest, and discard unused
>   * areas.
> diff --git a/qga/main.c b/qga/main.c
> index 0e04e73..8bcedaf 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -34,6 +34,10 @@
>  #include "qemu/bswap.h"
>  #ifdef _WIN32
>  #include "qga/service-win32.h"
> +#ifdef HAS_VSS_SDK
> +#include "qga/vss-win32-provider.h"
> +#include "qga/vss-win32-requester.h"
> +#endif

The protection of #include "qga/vss-win32-requester.h" is inconsistent
between "commands-win32.c" and "qga/main.c". For now the header only
brings in a few prototypes, which shouldn't cause trouble in
"commands-win32.c" even without VSS. Still consistent guarding would be
nice.


>  #include <windows.h>
>  #endif
>  #ifdef __linux__
> @@ -701,6 +705,25 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
>  }
>  
>  #ifdef _WIN32
> +
> +static gboolean vss_win32_init(void)
> +{
> +#ifdef HAS_VSS_SDK
> +    if (FAILED(vss_init())) {
> +        g_critical("failed to initialize VSS");
> +        return false;
> +    }
> +#endif
> +    return true;
> +}
> +
> +static void vss_win32_deinit(void)
> +{
> +#ifdef HAS_VSS_SDK
> +    vss_deinit();
> +#endif
> +}
> +
>  DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
>                                    LPVOID ctx)
>  {
> @@ -743,8 +766,12 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
>      service->status.dwWaitHint = 0;
>      SetServiceStatus(service->status_handle, &service->status);
>  
> +    if (!vss_win32_init()) {
> +        goto out_bad;
> +    }
>      g_main_loop_run(ga_state->main_loop);
> -
> +    vss_win32_deinit();
> +out_bad:
>      service->status.dwCurrentState = SERVICE_STOPPED;
>      SetServiceStatus(service->status_handle, &service->status);
>  }
> @@ -1175,7 +1202,11 @@ int main(int argc, char **argv)
>              { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
>          StartServiceCtrlDispatcher(service_table);
>      } else {
> +        if (!vss_win32_init()) {
> +            goto out_bad;
> +        }
>          g_main_loop_run(ga_state->main_loop);
> +        vss_win32_deinit();
>      }
>  #endif

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


... This patch made me look at ga_command_state_cleanup_all().
Apparently the POSIX flavor thaws filesystems if qemu-ga exits before a
thaw command over QMP; see guest_fsfreeze_cleanup() and
ga_command_state_init(). Do you think something similar would be useful
for the Windows flavor as well?

Thanks
Laszlo
Eric Blake - July 1, 2013, 5:48 p.m.
On 07/01/2013 07:29 AM, Laszlo Ersek wrote:
> some comments below
> 
> On 06/06/13 17:06, Tomoki Sekiyama wrote:
>> Support guest-fsfreeze-freeze and guest-fsfreeze-thaw commands for Windows
>> guests. When fsfreeze command is issued, it calls the VSS requester to
>> freeze filesystems and applications. On thaw command, it again tells the VSS
>> requester to thaw them.
>>

>>  int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>  {
>> +    int i;
>> +
>> +    if (!vss_initialized()) {
>> +        error_set(err, QERR_UNSUPPORTED);
>> +        return 0;
>> +    }
>> +
>> +    qga_vss_fsfreeze_thaw(&i, err);
> 
> I wonder how libvirt interprets a failure to thaw -- does it expect
> filesystems to remain frozen? (CC'ing Eric.)

On failure to thaw, libvirt marks the virDomainSnapshotCreateXML() API
call as failed, and logs the failure returned from the guest agent; but
beyond that, there really isn't anything further that libvirt can
attempt.  Guest freeze/thaw is a best-effort attempt, and we don't
really have any clean way to recover from a non-cooperative guest (ie.
it's no different from a Linux guest managing to kill off the guest
agent daemon in the guest at the wrong time).  In short, a failed
snapshot request has no guarantees on whether the guest is frozen or
thawed, and libvirt currently has no additional support to try a thaw in
isolation (although there is an intentionally unsupported 'virsh
qemu-agent-command' for attempting recovery manually).

> ... This patch made me look at ga_command_state_cleanup_all().
> Apparently the POSIX flavor thaws filesystems if qemu-ga exits before a
> thaw command over QMP; see guest_fsfreeze_cleanup() and
> ga_command_state_init(). Do you think something similar would be useful
> for the Windows flavor as well?

I agree that it is better to try and clean up a thaw on any error case
(including the error of qemu-ga exiting without seeing a QMP thaw
request), if only to minimize the chance of failure leaving the system
wedged with no chance of further recovery attempts.
Tomoki Sekiyama - July 1, 2013, 5:59 p.m.
On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:

>>+error:
>> +    qmp_guest_fsfreeze_thaw(NULL);
>
>Passing NULL here as "errp" concerns me slightly. I've been assuming
>that "errp" is never NULL due to the outermost QMP layer always wanting
>to receive errors and to serialize them.
>
>Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>questions would answer with false. That said, nothing seems to be
>affected right now.
>
>Maybe a dummy local variable would be more future-proof... OTOH it would
>be stylistically questionable :)

OK, then it should be:
    Error *local_err = NULL;
...
error:
    qmp_guest_fsfreeze_thaw(&local_err);
    if (error_is_set(&local_err)) {
        error_free(local_err);
    }

>>+#ifdef HAS_VSS_SDK
>> +#include "qga/vss-win32-provider.h"
>> +#include "qga/vss-win32-requester.h"
>> +#endif
>
>The protection of #include "qga/vss-win32-requester.h" is inconsistent
>between "commands-win32.c" and "qga/main.c". For now the header only
>brings in a few prototypes, which shouldn't cause trouble in
>"commands-win32.c" even without VSS. Still consistent guarding would be
>nice.

I will remove #ifdef's from qga/main.c. Instead, I will add static inline
functions that does nothing when CONFIG_VSS_SDK isn't defined.
(And vss-win32-provider.h can also be removed by stop linking the DLL.)


>... This patch made me look at ga_command_state_cleanup_all().
>Apparently the POSIX flavor thaws filesystems if qemu-ga exits before a
>thaw command over QMP; see guest_fsfreeze_cleanup() and
>ga_command_state_init(). Do you think something similar would be useful
>for the Windows flavor as well?

Hm, however the backup is automatically terminated if IVssBackupComponents
is released, it would be nice to have cleanup. I will add them.

Thanks,
Tomoki Sekiyama
Laszlo Ersek - July 2, 2013, 6:36 a.m.
On 07/01/13 19:59, Tomoki Sekiyama wrote:
> On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
> 
>>> +error:
>>> +    qmp_guest_fsfreeze_thaw(NULL);
>>
>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>> that "errp" is never NULL due to the outermost QMP layer always wanting
>> to receive errors and to serialize them.
>>
>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>> questions would answer with false. That said, nothing seems to be
>> affected right now.
>>
>> Maybe a dummy local variable would be more future-proof... OTOH it would
>> be stylistically questionable :)
> 
> OK, then it should be:
>     Error *local_err = NULL;
> ...
> error:
>     qmp_guest_fsfreeze_thaw(&local_err);
>     if (error_is_set(&local_err)) {
>         error_free(local_err);
>     }

I think so, yes.

You could also log it before freeing it I guess:

    g_debug("cleanup thaw: %s", error_get_pretty(local_err));

or some such, but it's probably not important.

Thanks,
Laszlo
Luiz Capitulino - July 2, 2013, 2:09 p.m.
On Tue, 02 Jul 2013 08:36:11 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/01/13 19:59, Tomoki Sekiyama wrote:
> > On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
> > 
> >>> +error:
> >>> +    qmp_guest_fsfreeze_thaw(NULL);
> >>
> >> Passing NULL here as "errp" concerns me slightly. I've been assuming
> >> that "errp" is never NULL due to the outermost QMP layer always wanting
> >> to receive errors and to serialize them.
> >>
> >> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
> >> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
> >> questions would answer with false. That said, nothing seems to be
> >> affected right now.
> >>
> >> Maybe a dummy local variable would be more future-proof... OTOH it would
> >> be stylistically questionable :)
> > 
> > OK, then it should be:
> >     Error *local_err = NULL;
> > ...
> > error:
> >     qmp_guest_fsfreeze_thaw(&local_err);
> >     if (error_is_set(&local_err)) {
> >         error_free(local_err);
> >     }
> 
> I think so, yes.
> 
> You could also log it before freeing it I guess:
> 
>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
> 
> or some such, but it's probably not important.

I'd rather do something like that, otherwise it doesn't seem to
make sense to pass Error as qmp_guest_fsfreeze_thaw() does
use a local Error.
Laszlo Ersek - July 2, 2013, 2:44 p.m.
On 07/02/13 16:09, Luiz Capitulino wrote:
> On Tue, 02 Jul 2013 08:36:11 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>>> On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
>>>
>>>>> +error:
>>>>> +    qmp_guest_fsfreeze_thaw(NULL);
>>>>
>>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>>>> that "errp" is never NULL due to the outermost QMP layer always wanting
>>>> to receive errors and to serialize them.
>>>>
>>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>>>> questions would answer with false. That said, nothing seems to be
>>>> affected right now.
>>>>
>>>> Maybe a dummy local variable would be more future-proof... OTOH it would
>>>> be stylistically questionable :)
>>>
>>> OK, then it should be:
>>>     Error *local_err = NULL;
>>> ...
>>> error:
>>>     qmp_guest_fsfreeze_thaw(&local_err);
>>>     if (error_is_set(&local_err)) {
>>>         error_free(local_err);
>>>     }
>>
>> I think so, yes.
>>
>> You could also log it before freeing it I guess:
>>
>>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
>>
>> or some such, but it's probably not important.
> 
> I'd rather do something like that, otherwise it doesn't seem to
> make sense to pass Error as qmp_guest_fsfreeze_thaw() does
> use a local Error.

No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
patch doesn't.

Thanks,
Laszlo
Michael Roth - July 2, 2013, 2:58 p.m.
On Tue, Jul 2, 2013 at 1:36 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>> On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
>>
>>>> +error:
>>>> +    qmp_guest_fsfreeze_thaw(NULL);
>>>
>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>>> that "errp" is never NULL due to the outermost QMP layer always wanting
>>> to receive errors and to serialize them.
>>>
>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>>> questions would answer with false. That said, nothing seems to be
>>> affected right now.
>>>
>>> Maybe a dummy local variable would be more future-proof... OTOH it would
>>> be stylistically questionable :)
>>
>> OK, then it should be:
>>     Error *local_err = NULL;
>> ...
>> error:
>>     qmp_guest_fsfreeze_thaw(&local_err);
>>     if (error_is_set(&local_err)) {
>>         error_free(local_err);
>>     }
>
> I think so, yes.
>
> You could also log it before freeing it I guess:
>
>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
>
> or some such, but it's probably not important.

One thing to keep in mind is there are some failure paths in
qmp_guest_fsfreeze_thaw(), for both win32 and posix, where we might not unset
frozen state via ga_set_unfrozen(). In which case, logging will still be
disabled.

It might make sense to nest those error strings inside the one returned by
qmp_guest_fsfreeze_freeze(), since that's the only reliable way to report it
and the client will be interested in that information. This also makes
introducing local_err immediately useful.

>
> Thanks,
> Laszlo
>
Luiz Capitulino - July 2, 2013, 3:16 p.m.
On Tue, 02 Jul 2013 16:44:55 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/02/13 16:09, Luiz Capitulino wrote:
> > On Tue, 02 Jul 2013 08:36:11 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> >> On 07/01/13 19:59, Tomoki Sekiyama wrote:
> >>> On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
> >>>
> >>>>> +error:
> >>>>> +    qmp_guest_fsfreeze_thaw(NULL);
> >>>>
> >>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
> >>>> that "errp" is never NULL due to the outermost QMP layer always wanting
> >>>> to receive errors and to serialize them.
> >>>>
> >>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
> >>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
> >>>> questions would answer with false. That said, nothing seems to be
> >>>> affected right now.
> >>>>
> >>>> Maybe a dummy local variable would be more future-proof... OTOH it would
> >>>> be stylistically questionable :)
> >>>
> >>> OK, then it should be:
> >>>     Error *local_err = NULL;
> >>> ...
> >>> error:
> >>>     qmp_guest_fsfreeze_thaw(&local_err);
> >>>     if (error_is_set(&local_err)) {
> >>>         error_free(local_err);
> >>>     }
> >>
> >> I think so, yes.
> >>
> >> You could also log it before freeing it I guess:
> >>
> >>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
> >>
> >> or some such, but it's probably not important.
> > 
> > I'd rather do something like that, otherwise it doesn't seem to
> > make sense to pass Error as qmp_guest_fsfreeze_thaw() does
> > use a local Error.
> 
> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
> patch doesn't.

Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw()
that has to be fixed, isn't it?
Laszlo Ersek - July 2, 2013, 3:28 p.m.
On 07/02/13 17:16, Luiz Capitulino wrote:
> On Tue, 02 Jul 2013 16:44:55 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 07/02/13 16:09, Luiz Capitulino wrote:
>>> On Tue, 02 Jul 2013 08:36:11 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>>>>> On 7/1/13 9:29 , "Laszlo Ersek" <lersek@redhat.com> wrote:
>>>>>
>>>>>>> +error:
>>>>>>> +    qmp_guest_fsfreeze_thaw(NULL);
>>>>>>
>>>>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>>>>>> that "errp" is never NULL due to the outermost QMP layer always wanting
>>>>>> to receive errors and to serialize them.
>>>>>>
>>>>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>>>>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>>>>>> questions would answer with false. That said, nothing seems to be
>>>>>> affected right now.
>>>>>>
>>>>>> Maybe a dummy local variable would be more future-proof... OTOH it would
>>>>>> be stylistically questionable :)
>>>>>
>>>>> OK, then it should be:
>>>>>     Error *local_err = NULL;
>>>>> ...
>>>>> error:
>>>>>     qmp_guest_fsfreeze_thaw(&local_err);
>>>>>     if (error_is_set(&local_err)) {
>>>>>         error_free(local_err);
>>>>>     }
>>>>
>>>> I think so, yes.
>>>>
>>>> You could also log it before freeing it I guess:
>>>>
>>>>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
>>>>
>>>> or some such, but it's probably not important.
>>>
>>> I'd rather do something like that, otherwise it doesn't seem to
>>> make sense to pass Error as qmp_guest_fsfreeze_thaw() does
>>> use a local Error.
>>
>> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
>> patch doesn't.
> 
> Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw()
> that has to be fixed, isn't it?

I didn't insist on that because the QMP command implementations in the
guest agent are all rooted in the dispatcher, and the dispatcher makes
sure for all commands that errp is never NULL. This is the only call
site where a NULL errp was manually passed, and we're discussing how to
fix it -- use a local_err, and maybe even print it.

Of course I don't insist on *not* reworking it either.

Thanks,
Laszlo

Patch

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 24e4ad0..67dca60 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -15,6 +15,7 @@ 
 #include <wtypes.h>
 #include <powrprof.h>
 #include "qga/guest-agent-core.h"
+#include "qga/vss-win32-requester.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
 
@@ -151,34 +152,95 @@  void qmp_guest_file_flush(int64_t handle, Error **err)
     error_set(err, QERR_UNSUPPORTED);
 }
 
+#ifdef HAS_VSS_SDK
+
 /*
  * Return status of freeze/thaw
  */
 GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
 {
-    error_set(err, QERR_UNSUPPORTED);
-    return 0;
+    if (!vss_initialized()) {
+        error_set(err, QERR_UNSUPPORTED);
+        return 0;
+    }
+
+    if (ga_is_frozen(ga_state)) {
+        return GUEST_FSFREEZE_STATUS_FROZEN;
+    }
+
+    return GUEST_FSFREEZE_STATUS_THAWED;
 }
 
 /*
- * Walk list of mounted file systems in the guest, and freeze the ones which
- * are real local file systems.
+ * Freeze local file systems using Volume Shadow-copy Service.
+ * The frozen state is limited for up to 10 seconds by VSS.
  */
 int64_t qmp_guest_fsfreeze_freeze(Error **err)
 {
-    error_set(err, QERR_UNSUPPORTED);
+    int i;
+
+    slog("guest-fsfreeze called");
+
+    if (!vss_initialized()) {
+        error_set(err, QERR_UNSUPPORTED);
+        return 0;
+    }
+
+    /* cannot risk guest agent blocking itself on a write in this state */
+    ga_set_frozen(ga_state);
+
+    qga_vss_fsfreeze_freeze(&i, err);
+    if (error_is_set(err)) {
+        goto error;
+    }
+
+    return i;
+
+error:
+    qmp_guest_fsfreeze_thaw(NULL);
     return 0;
 }
 
 /*
- * Walk list of frozen file systems in the guest, and thaw them.
+ * Thaw local file systems using Volume Shadow-copy Service.
  */
 int64_t qmp_guest_fsfreeze_thaw(Error **err)
 {
+    int i;
+
+    if (!vss_initialized()) {
+        error_set(err, QERR_UNSUPPORTED);
+        return 0;
+    }
+
+    qga_vss_fsfreeze_thaw(&i, err);
+
+    ga_unset_frozen(ga_state);
+    return i;
+}
+
+#else
+
+GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
+{
+    error_set(err, QERR_UNSUPPORTED);
+    return 0;
+}
+
+int64_t qmp_guest_fsfreeze_freeze(Error **err)
+{
+    error_set(err, QERR_UNSUPPORTED);
+    return 0;
+}
+
+int64_t qmp_guest_fsfreeze_thaw(Error **err)
+{
     error_set(err, QERR_UNSUPPORTED);
     return 0;
 }
 
+#endif
+
 /*
  * Walk list of mounted file systems in the guest, and discard unused
  * areas.
diff --git a/qga/main.c b/qga/main.c
index 0e04e73..8bcedaf 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -34,6 +34,10 @@ 
 #include "qemu/bswap.h"
 #ifdef _WIN32
 #include "qga/service-win32.h"
+#ifdef HAS_VSS_SDK
+#include "qga/vss-win32-provider.h"
+#include "qga/vss-win32-requester.h"
+#endif
 #include <windows.h>
 #endif
 #ifdef __linux__
@@ -701,6 +705,25 @@  static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
 }
 
 #ifdef _WIN32
+
+static gboolean vss_win32_init(void)
+{
+#ifdef HAS_VSS_SDK
+    if (FAILED(vss_init())) {
+        g_critical("failed to initialize VSS");
+        return false;
+    }
+#endif
+    return true;
+}
+
+static void vss_win32_deinit(void)
+{
+#ifdef HAS_VSS_SDK
+    vss_deinit();
+#endif
+}
+
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx)
 {
@@ -743,8 +766,12 @@  VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
     service->status.dwWaitHint = 0;
     SetServiceStatus(service->status_handle, &service->status);
 
+    if (!vss_win32_init()) {
+        goto out_bad;
+    }
     g_main_loop_run(ga_state->main_loop);
-
+    vss_win32_deinit();
+out_bad:
     service->status.dwCurrentState = SERVICE_STOPPED;
     SetServiceStatus(service->status_handle, &service->status);
 }
@@ -1175,7 +1202,11 @@  int main(int argc, char **argv)
             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
         StartServiceCtrlDispatcher(service_table);
     } else {
+        if (!vss_win32_init()) {
+            goto out_bad;
+        }
         g_main_loop_run(ga_state->main_loop);
+        vss_win32_deinit();
     }
 #endif