Re: [Qemu-devel] [PATCH v4 07/10] qemu-ga: Add Windows VSS requester to quiesce applications and filesystems

[Laszlo Ersek]

On 06/06/13 17:06, Tomoki Sekiyama wrote:

> diff --git a/qga/vss-win32-requester.h b/qga/vss-win32-requester.h
> new file mode 100644
> index 0000000..f180f56
> --- /dev/null
> +++ b/qga/vss-win32-requester.h
> @@ -0,0 +1,31 @@
> +/*
> + * QEMU Guest Agent VSS Requester declarations
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef VSS_WIN32_REQUESTER_H
> +#define VSS_WIN32_REQUESTER_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +HRESULT vss_init(void);

Can you include the mingw header that defines the HRESULT type? As far
as I know we like to make headers standalone.

(OTOH I vaguely recall a patch where the order between a mingw header
and a (generated?) trace header could cause a build error... I think it
would be worth trying still.)


> +void vss_deinit(void);
> +int vss_initialized(void);

Since this is qemu-ga / qemu code, I think a "bool" return type would be
more usual. (The current prototype is correct too of course.)


> +
> +void qga_vss_fsfreeze_freeze(int *nr_volume, struct Error **err);
> +void qga_vss_fsfreeze_thaw(int *nr_volume, struct Error **err);

Can you drop the "struct" word in these prototypes?


> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
>
>

> diff --git a/qga/vss-win32-requester.cpp b/qga/vss-win32-requester.cpp
> new file mode 100644
> index 0000000..7784926
> --- /dev/null
> +++ b/qga/vss-win32-requester.cpp
> @@ -0,0 +1,419 @@
> +/*
> + * QEMU Guest Agent win32 VSS Requester implementations
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +#include <assert.h>

Can you remove this #include and replace all assert() occurrences with
g_assert()?


> +extern "C" {
> +#include "guest-agent-core.h"
> +}
> +#include "vss-win32-requester.h"
> +#include "vss-win32-provider.h"
> +#include "vss-win32.h"
> +#include "inc/win2003/vswriter.h"
> +#include "inc/win2003/vsbackup.h"
> +
> +/* Functions in VSSAPI.DLL */
> +typedef HRESULT(STDAPICALLTYPE * t_CreateVssBackupComponents)(
> +    OUT IVssBackupComponents**);
> +typedef void(APIENTRY * t_VssFreeSnapshotProperties)(IN VSS_SNAPSHOT_PROP*);
> +
> +static t_CreateVssBackupComponents _CreateVssBackupComponents;
> +static t_VssFreeSnapshotProperties _VssFreeSnapshotProperties;

I apologize in advance for splitting hairs, but :)

  17.4.3.1.2 Global names [lib.global.names]; p1

  Certain sets of names and function signatures are always reserved to
  the implementation:

  - Each name that contains a double underscore (__) or begins with an
    underscore followed by an uppercase letter (2.11) is reserved to the
    implementation for any use.

Unless there's a pressing reason, could you drop the leading
underscores?


> +static IVssBackupComponents *pVssbc;
> +static IVssAsync *pAsyncSnapshot;
> +static HMODULE hLib;
> +static HANDLE hEvent = INVALID_HANDLE_VALUE, hEvent2 = INVALID_HANDLE_VALUE;
> +static int cFrozenVols;

Can you decorate each of these static variables with a short comment? It
does get clear(er) what they are used for by reading the code, but the
comments would save others time.

Also I'd recommend grouping them differently:

- first group: long term objects related to VSSAPI.DLL and released
  *only* in vss_deinit(): hLib, _CreateVssBackupComponents,
  _VssFreeSnapshotProperties;

- second group: objects that make sense only in preparation for, during,
  and right after a freeze: pVssbc, pAsyncSnapshot, hEvent, hEvent2,
  cFrozenVols. (You could even introduce a struct for these, but that's
  just an idea.)


> +
> +GCC_FMT_ATTR(1, 2)
> +static void errmsg(const char *fmt, ...)
> +{
> +    va_list ap;
> +    va_start(ap, fmt);
> +    char *msg = g_strdup_vprintf(fmt, ap);
> +    va_end(ap);
> +    MessageBox(NULL, msg, "Error in QEMU guest agent", MB_OK | 
> MB_ICONWARNING);
> +    g_free(msg);
> +}

(Still splitting hairs, bear with me please:) can you rename this
function to errmsg_dialog() or something similar, for consistency with
06/10?


> +
> +static void error_set_win32(Error **errp, DWORD err,
> +                            ErrorClass eclass, const char *text)
> +{
> +    char *msg = NULL, *nul = strchr(text, '(');
> +    int len = nul ? nul - text : -1;
> +
> +    /* print error message in native encoding */
> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
> +                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                  (char *)&msg, 0, NULL);
> +    printf("%.*s. (Error: %lx) %s\n", len, text, err, msg);
> +    LocalFree(msg);

I don't think you should print anything here. error_set*() functions
just set the error when asked by a function F1. It is up to F1's caller,
let's call it F2, to propagate or to consume (= print to stderr or to
the monitor) the error received.

Errors encountered when processing VSS freeze/thaw requests should be
returned to the QMP caller like any other command error.

> +
> +    /* set error message in UTF-8 encoding */
> +    msg = g_win32_error_message(err);

Can we pass "err" (which is a HRESULT === DWORD === "long unsigned") to
g_win32_error_message()? The latter takes an "int".

MSDN seems to imply there are different "error code spaces", see
HRESULT_FROM_WIN32(). Anyway I'll assume we can do this.


> +    error_set(errp, eclass, "%.*s. (Error: %lx) %s", len, text, err, msg);
> +    g_free(msg);
> +}
> +#define error_setg_win32(errp, err, text) \
> +    error_set_win32(errp, err, ERROR_CLASS_GENERIC_ERROR, text)

This approach is fine in general for the VSS DLL I think, but for
qemu-ga.exe, I would propose a more flexible interface (could even
belong into "util/error.c", in an #ifdef _WIN32 block). It would imitate
error_setg_errno() in that it allowed the caller to pass in a format
string as well:

(a) Create a copy of error_set_errno() with "errno"/"strerror" replaced
by "win32"/"g_win32_error_message":

    void error_set_win32(Error **errp, int win32, ErrorClass err_class,
                         const char *fmt, ...)
    {
        Error *err;
        char *msg1;
        va_list ap;

        if (errp == NULL) {
            return;
        }
        assert(*errp == NULL);

        err = g_malloc0(sizeof(*err));

        va_start(ap, fmt);
        msg1 = g_strdup_vprintf(fmt, ap);
        if (win32 != 0) {
            char *msg2 = g_win32_error_message(win32);

            err->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
                                       (unsigned)win32);
            g_free(msg2);
            g_free(msg1);
        } else {
            err->msg = msg1;
        }
        va_end(ap);
        err->err_class = err_class;

        *errp = err;
    }

(b) error_setg_win32() should imitate error_setg_errno():

    #define error_setg_win32(err, win32, fmt, ...) \
        error_set_errno(err, win32, ERROR_CLASS_GENERIC_ERROR, fmt, ## 
__VA_ARGS__)


This is just a suggestion, but it would bring your code closer to "qemu
coding style", and render free-form error messages more convenient for
you (eg. you could drop the snprintf() stuff in
qga_vss_fsfreeze_freeze()).


> +
> +#define __chk(status, text, errp, err_label)    \
> +    do {                                        \
> +        HRESULT __hr = (status);                \
> +        if (FAILED(__hr)) {                     \
> +            error_setg_win32(errp, __hr, text); \
> +            goto err_label;                     \
> +        }                                       \
> +    } while (0)
> +
> +#define _chk(status, msg) __chk(status, "Failed to " msg, err, out)
> +#define chk(status)  __chk(status, "Failed to " #status, err, out)

I'd prefer if you hand-expanded these macros in qemu-ga code. The
error_setg_win32() version suggested above should make it easy to format
any error message. You'd have to open-code the "goto" and the specific
label naturally, but that would also match the rest of qemu code better
IMHO. (More work to write once, but easier to read many times.)


> +
> +
> +HRESULT WaitForAsync(IVssAsync *pAsync)
> +{
> +    HRESULT ret, hr;
> +
> +    do {
> +        hr = pAsync->Wait();
> +        if (FAILED(hr)) {
> +            ret = hr;
> +            break;
> +        }
> +        hr = pAsync->QueryStatus(&ret, NULL);
> +        if (FAILED(hr)) {
> +            ret = hr;
> +            break;
> +        }
> +    } while (ret == VSS_S_ASYNC_PENDING);
> +
> +    return ret;
> +}

Hm. Are we expecting spurious wakeups in Wait()? MSDN sayeth if Wait()
returns with S_OK, then the async op completed, and QueryStatus() will
return the *final* status of the async op (emphasis mine). Meaning, I
would think, VSS_S_ASYNC_CANCELLED or VSS_S_ASYNC_FINISHED.

Anyhow, the function appears correct.


> +
> +HRESULT vss_init(void)
> +{
> +    HRESULT hr;
> +
> +    hr = VSSCheckOSVersion();
> +    if (hr == S_FALSE) {
> +        return hr;
> +    }

Ah, I think you may have addressed this already -- this is the reason
"vss-win32-provider/install.o" is needed when linking qemu-ga.exe.

VSSCheckOSVersion() is quite small, relies on no C++ *or* VSS features,
and is arguably a utility function. Can you move it to one of:
- util/osdep.c,
- os-win32.c,
- util/oslib-win32.c?

(I'm not sure exactly which one would be best, but I guess Paolo can tell.)

Furthermore, if VSSCheckOSVersion() fails, we should notify the user
(based at least on the fact that other errors in vss_init() get
reported). In "install.cpp" too you report VSSCheckOSVersion() failures
with printf().


> +
> +    hr = CoInitialize(NULL);
> +    if (FAILED(hr)) {
> +        errmsg("CoInitialize failed [%lx]", hr);
> +        goto out;
> +    };

>From what you said recently, CoInitialize() can't fail.

Independently, is errmsg() -- or as I'm proposing, errmsg_dialog() --
justified here? (I don't know about the context(s) yet you're going to
call vss_init() from.)


> +    hr = CoInitializeSecurity(
> +        NULL, -1, NULL, NULL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
> +        RPC_C_IMP_LEVEL_IDENTIFY, NULL, EOAC_NONE, NULL);

I'll trust you on all these parameters... There's only 9 of them if I'm
counting right :)


> +    if (FAILED(hr)) {
> +        errmsg("CoInitializeSecurity failed [%lx]", hr);
> +        goto out;
> +    }
> +
> +    hLib = LoadLibraryA("VSSAPI.DLL");
> +    if (!hLib) {
> +        errmsg("LoadLibrary VSSAPI.DLL failed");
> +        hr = E_FAIL;
> +        goto out;
> +    }

(I presume CoUninitialize() will undo CoInitializeSecurity()'s effects
as well.)


> +
> +    _CreateVssBackupComponents = (t_CreateVssBackupComponents)
> +        GetProcAddress(hLib,
> +#ifdef _WIN64 /* 64bit environment */
> +        "?CreateVssBackupComponents@@YAJPEAPEAVIVssBackupComponents@@@Z"
> +#else /* 32bit environment */
> +        "?CreateVssBackupComponents@@YGJPAPAVIVssBackupComponents@@@Z"
> +#endif
> +        );
> +    _VssFreeSnapshotProperties = (t_VssFreeSnapshotProperties)
> +        GetProcAddress(hLib, "VssFreeSnapshotProperties");
> +    if (!_CreateVssBackupComponents || !_VssFreeSnapshotProperties) {
> +        errmsg("GetProcAddress failed");
> +        hr = E_FAIL;
> +        goto out;
> +    }
> +
> +    return S_OK;
> +out:
> +    vss_deinit();
> +    return hr;
> +}

OK.

> +
> +static void vss_cleanup(void)
> +{
> +    if (hEvent != INVALID_HANDLE_VALUE) {
> +        CloseHandle(hEvent);
> +        hEvent = INVALID_HANDLE_VALUE;
> +    }
> +    if (hEvent2 != INVALID_HANDLE_VALUE) {
> +        CloseHandle(hEvent2);
> +        hEvent2 = INVALID_HANDLE_VALUE;
> +    }
> +    if (pVssbc) {
> +        pVssbc->Release();
> +        pVssbc = NULL;
> +    }
> +}
> +
> +void vss_deinit(void)
> +{
> +    if (VSSCheckOSVersion() == S_FALSE) {
> +        return;
> +    }
> +
> +    vss_cleanup();
> +
> +    CoUninitialize();
> +
> +    _CreateVssBackupComponents = NULL;
> +    _VssFreeSnapshotProperties = NULL;
> +    if (hLib) {
> +        FreeLibrary(hLib);
> +        hLib = NULL;
> +    }
> +}

This separation of cleanup tasks seems apt (peeking forward a bit).

However, from all the static(-ally referenced) data, "pAsyncSnapshot" is
not handled in vss_cleanup(). Peeking forward a bit again, I think that
"pAsyncSnapshot" might be leaked if the loop in
qga_vss_fsfreeze_freeze() terminates with a QueryStatus() error.


> +
> +int vss_initialized(void)
> +{
> +    return hLib != NULL;
> +}
> +
> +static void vss_add_components(Error **err)
> +{
> +    unsigned int cWriters, i;
> +    VSS_ID id, idInstance, idWriter;
> +    BSTR bstrWriterName;
> +    VSS_USAGE_TYPE usage;
> +    VSS_SOURCE_TYPE source;
> +    unsigned int cComponents, c1, c2, j;
> +    IVssExamineWriterMetadata *pMetadata;
> +    IVssWMComponent *pComponent;
> +    PVSSCOMPONENTINFO pInfo = NULL;
> +
> +    chk(pVssbc->GetWriterMetadataCount(&cWriters));

This could jump to "out" with an indeterminate "pComponent" (and call
pComponent->Release()).

I think the chk() macro makes a false promise -- you can't avoid
examining error conditions individually. (Eliminating chk() & co., as
I've suggested, should help with this too.)

"pMetadata" too is indeterminate here.

> +
> +    for (i = 0; i < cWriters; i++) {
> +        chk(pVssbc->GetWriterMetadata(i, &id, &pMetadata));
> +        chk(pMetadata->GetIdentity(&idInstance, &idWriter,
> +                                    &bstrWriterName, &usage, &source));
> +        chk(pMetadata->GetFileCounts(&c1, &c2, &cComponents));
> +
> +        for (j = 0; j < cComponents; j++) {
> +            chk(pMetadata->GetComponent(j, &pComponent));
> +            chk(pComponent->GetComponentInfo(&pInfo));
> +            if (pInfo->bSelectable) {
> +                chk(pVssbc->AddComponent(idInstance, idWriter, pInfo->type,
> +                                         pInfo->bstrLogicalPath,
> +                                         pInfo->bstrComponentName));
> +            }
> +            pComponent->FreeComponentInfo(pInfo);
> +            pInfo = NULL;
> +            pComponent->Release();
> +            pComponent = NULL;
> +        }
> +
> +        pMetadata->Release();
> +        pMetadata = NULL;

Should we free "bstrWriterName" too? (If so, then we must check it under
"out" as well I guess.)


> +    }
> +out:
> +    if (pComponent) {
> +        if (pInfo) {
> +            pComponent->FreeComponentInfo(pInfo);
> +        }
> +        pComponent->Release();
> +    }
> +    if (pMetadata) {
> +        pMetadata->Release();
> +    }
> +}

Presumably, if this function (or something after it) fails, any
components added with pVssbc->AddComponent() will be removed by
pVssbc->AbortBackup() or pVssbc->Release() (in vss_cleanup()) on the
error path in qga_vss_fsfreeze_freeze().

> +
> +void qga_vss_fsfreeze_freeze(int *num_vols, Error **err)
> +{
> +    IVssAsync *pAsync;
> +    HANDLE h;
> +    HRESULT hr;
> +    LONG ctx;
> +    GUID guidSnapshotSet = GUID_NULL;
> +    SECURITY_DESCRIPTOR sd;
> +    SECURITY_ATTRIBUTES sa;
> +    WCHAR buf[64], *b = buf;
> +    int n = 0;
> +
> +    if (pVssbc) { /* already frozen */
> +        *num_vols = 0;
> +        return;
> +    }

The Linux / POSIX implementation makes calls to ga_set_frozen() /
ga_unset_frozen(). ga_set_frozen() temporarily disables some qga
commands (that would require a thawed filesystem to work), plus it
creates an isfrozen file in (IIRC) the state directory. The isfrozen
file serves as "persistent reminder" for the next qga startup, should
qga die while the system is frozen.

I think *with time* we should investigate how / if ga_set_frozen() can
be applied to the windows implementation of fsfreeze. However I don't
think we should block this series until that time (feel free to disagree
though and implement that call too if you wish!)


> +
> +    assert(_CreateVssBackupComponents != NULL);

(g_assert())

> +    chk(_CreateVssBackupComponents(&pVssbc));
> +    chk(pVssbc->InitializeForBackup());

Hopefully AbortBackup() is valid for "pVssbc" even when
InitializeForBackup() fails first.


> +    chk(pVssbc->SetBackupState(true, true, VSS_BT_FULL, false));
> +    /*
> +     * Currently writable snapshots are not supported.
> +     * To prevent the final commit (which requires to write to snapshots),
> +     * ATTR_NO_AUTORECOVERY and ATTR_TRANSPORTABLE are specified here.
> +     */
> +    ctx = VSS_CTX_APP_ROLLBACK | VSS_VOLSNAP_ATTR_TRANSPORTABLE |
> +        VSS_VOLSNAP_ATTR_NO_AUTORECOVERY | VSS_VOLSNAP_ATTR_TXF_RECOVERY;
> +    hr = pVssbc->SetContext(ctx);
> +    if (hr == (HRESULT)VSS_E_UNSUPPORTED_CONTEXT) {
> +        /* Non-server version of Windows doesn't support ATTR_TRANSPORTABLE 
> */
> +        ctx &= ~VSS_VOLSNAP_ATTR_TRANSPORTABLE;
> +        chk(pVssbc->SetContext(ctx));
> +    } else {
> +        _chk(hr, "SetContext");
> +    }
> +
> +    chk(pVssbc->GatherWriterMetadata(&pAsync));
> +    _chk(WaitForAsync(pAsync), "GatherWriterMetadata");

If WaitForAsync() fails, this leaks pAsync.

> +    pAsync->Release();

OTOH once you start handling pAsync under "out", you'll need to null it
here.

> +
> +    vss_add_components(err);
> +    if (error_is_set(err)) {
> +        goto out;
> +    }
> +
> +    chk(pVssbc->StartSnapshotSet(&guidSnapshotSet));
> +
> +    h = FindFirstVolumeW(buf, sizeof(buf));
> +    while (h != INVALID_HANDLE_VALUE) {

"h" doesn't appear to change in the loop. Hence I'd prefer an "if" after
FindFirstVolumeW(), and a for (;;) here.

> +        if (GetDriveTypeW(buf) == DRIVE_FIXED) {
> +            VSS_ID pid;
> +            hr = pVssbc->AddToSnapshotSet(buf, g_gProviderId, &pid);
> +            if (FAILED(hr)) {
> +                WCHAR name[PATH_MAX];
> +                char msg[PATH_MAX+32];
> +                if (GetVolumePathNamesForVolumeNameW(
> +                        buf, name, sizeof(name), NULL) && *name) {
> +                    b = name;
> +                }
> +                snprintf(msg, sizeof(msg), "add %S to snapshot set", b);

(let's hope %S won't run into EILSEQ -- the same applies if you agree to
rebase this code to the proposed error_setg_win32())

> +                error_setg_win32(err, hr, msg);
> +                goto out;

You miss FindVolumeClose() here.


> +            }
> +            n++;

Can you rename
- "n" to "num_fixed_drives",
- "h" to "volume",
- "buf" to "short_volume_name",
- "name" to "volume_path_names"?


> +        }
> +        if (!FindNextVolumeW(h, buf, sizeof(buf))) {
> +            FindVolumeClose(h);
> +            break;
> +        }
> +    }

Just a suggestion: if "n" ("num_fixed_drives") is zero here, we could
short-circuit the fsfreeze command (no volume added to the snapshot
set). OTOH I can't imagine any windows installation without a fixed
drive, admittedly.

> +
> +    chk(pVssbc->PrepareForBackup(&pAsync));
> +    _chk(WaitForAsync(pAsync), "PrepareForBackup");
> +    pAsync->Release();

Again, we should release "pAsync" if WaitForAsync() fails, and null it
if WaitForAsync() succeeds.

> +
> +    chk(pVssbc->GatherWriterStatus(&pAsync));
> +    _chk(WaitForAsync(pAsync), "GatherWriterStatus");
> +    pAsync->Release();

ditto

> +
> +    /* Allow unrestricted access to events */
> +    InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION);
> +    SetSecurityDescriptorDacl(&sd, TRUE, NULL, FALSE);
> +    sa.nLength = sizeof(sa);
> +    sa.lpSecurityDescriptor = &sd;
> +    sa.bInheritHandle = FALSE;

(I'll assume "sd" and "sa" don't need cleanup.)

> +
> +    hEvent = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
> +    if (hEvent == INVALID_HANDLE_VALUE) {
> +        error_setg_win32(err, GetLastError(), "CreateEvenet");

typo in "CreateEvenet"

> +        goto out;
> +    }
> +    hEvent2 = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
> +    if (hEvent2 == INVALID_HANDLE_VALUE) {
> +        error_setg_win32(err, GetLastError(), "CreateEvenet");
> +        goto out;

The typo was replicated here.

> +    }
> +
> +    chk(pVssbc->DoSnapshotSet(&pAsyncSnapshot));

... The events are closed in vss_cleanup() on failure, OK.

Am I right to think that this is the point where VSS enters
CQGAVssProvider::CommitSnapshots()? In that case the ordering of the two
CreateEvent() calls vs. DoSnapshotSet() is correct, the OpenEvent()
calls in CQGAVssProvider::CommitSnapshots() will find the events.


> +
> +    /* Need to call QueryStatus several times to make VSS provider progress 
> */

Very strange!

> +    for (int i = 0; i < 1000; i++) {

What do you think of a macro for the repeat count?

> +        HRESULT hr = S_OK;

Is the initialization necessary? If QueryStatus() succeeds, "hr" should
be overwritten, should it not? Just asking because the initialization
implies we might read "hr" without any further assignment to it.

> +        chk(pAsyncSnapshot->QueryStatus(&hr, NULL));
> +        if (hr != VSS_S_ASYNC_PENDING) {
> +            error_setg(err, "DoSnapshotSet exited without freeze event");
> +            goto out;
> +        }

First I thought this was race-prone, but I was wrong -- when
CQGAVssProvider::CommitSnapshots() kicks the frozen event and blocks on
the thawed event, the status is still VSS_S_ASYNC_PENDING. OK.

Additionally, as I mentioned previously, if either chk() or the "hr"
check fails, we'll leak "pAsyncSnapshot".

> +        DWORD ret = WaitForSingleObject(hEvent, 10);
> +        if (ret == WAIT_OBJECT_0) {
> +            break;
> +        }

Should we look for other possible return values? For example,
WAIT_FAILED would render the rest of the loop moot.

> +    }

10,000 msecs for the loop seems fine.

But, should we check here, after the loop, whether "ret" equals
WAIT_OBJECT_0?

WAIT_TIMEOUT would imply i==1000, a real timeout. WAIT_FAILED would be
another error. (Of course the QMP caller would have no idea what
happened to VSS in the background, but that's just the nature of
timeouts.)

> +
> +    *num_vols = cFrozenVols = n;
> +    return;

Right. At this point we return to libvirt, from the fsfreeze command,
the file systems are frozen, and CQGAVssProvider::CommitSnapshots() is
blocking, for 60 seconds, on the "thaw" event.

> +
> +out:
> +    if (pVssbc) {
> +        pVssbc->AbortBackup();
> +    }
> +    vss_cleanup();
> +}
> +
> +
> +void qga_vss_fsfreeze_thaw(int *num_vols, Error **err)
> +{
> +    IVssAsync *pAsync;
> +
> +    if (hEvent2 == INVALID_HANDLE_VALUE) {
> +        /*
> +         * In this case, DoSnapshotSet is aborted or not started,
> +         * and no volumes must be frozen. We return without an error.
> +         */
> +        *num_vols = 0;
> +        return;
> +    }
> +    SetEvent(hEvent2);
> +
> +    assert(pVssbc);
> +    assert(pAsyncSnapshot);

g_assert(), but otherwise: correct. (hEvent2 != INVALID_HANDLE_VALUE)
implies both of these pointers are valid.

> +
> +    HRESULT hr = WaitForAsync(pAsyncSnapshot);
> +    if (hr == (HRESULT)VSS_E_OBJECT_NOT_FOUND) {
> +        /*
> +         * On Windows earlier than 2008 SP2 which does not support
> +         * VSS_VOLSNAP_ATTR_NO_AUTORECOVERY context, the final commit is not
> +         * skipped and VSS is aborted by VSS_E_OBJECT_NOT_FOUND. Still, as 
> the
> +         * applications and file systems are frozen, we just ignore the 
> error.

I think the comment is mis-worded, applications and file systems should
be thawed, not frozen, at this point, no?

> +         */
> +        pAsyncSnapshot->Release();
> +        pAsyncSnapshot = NULL;

As I've been parroting above :), these two steps should happen in
vss_cleanup() -- conditionally of course, like the rest there.

You could also move "cFrozenVols = 0" to that function (see the grouping
I recommended at the top).

> +        goto final;

Technically, the backup finished with an error. Should you not abort it,
like you do in case of other errors?

> +    }
> +    if (hr == (HRESULT)VSS_E_HOLD_WRITES_TIMEOUT) {
> +        _chk(hr, "DoSnapshotSet: Couldn't hold writes. "
> +             "Fsfreeze is limited up to 10 seconds");
> +    }
> +    _chk(hr, "DoSnapshotSet");
> +    pAsyncSnapshot->Release();
> +    pAsyncSnapshot = NULL;
> +
> +    chk(pVssbc->BackupComplete(&pAsync));
> +    _chk(WaitForAsync(pAsync), "BackupComplete");
> +    pAsync->Release();

This leaks pAsync if WaitForAsync fails.

> +
> +final:
> +    *num_vols = cFrozenVols;
> +    cFrozenVols = 0;
> +    goto done;
> +
> +out:
> +    if (pVssbc) {

"pVssbc" is never NULL here (see the assert, for example).

> +        pVssbc->AbortBackup();
> +    }
> +done:
> +    vss_cleanup();
> +}

For my taste, there are way too many complex goto's here. What do you
think of the following:

    void qga_vss_fsfreeze_thaw(int *num_vols, Error **err)
    {
        if (hEvent2 == INVALID_HANDLE_VALUE) {
            /*
             * In this case, DoSnapshotSet is aborted or not started,
             * and no volumes must be frozen. We return without an
             * error.
             */
            *num_vols = 0;
            return;
        }
        SetEvent(hEvent2);

        assert(pVssbc);
        assert(pAsyncSnapshot);

        HRESULT hr = WaitForAsync(pAsyncSnapshot);
        switch (hr) {
        case VSS_E_OBJECT_NOT_FOUND:
            /*
             * On Windows earlier than 2008 SP2 which does not support
             * VSS_VOLSNAP_ATTR_NO_AUTORECOVERY context, the final
             * commit is not skipped and VSS is aborted by
             * VSS_E_OBJECT_NOT_FOUND. Still, as the applications and
             * file systems are thawed, we just ignore the error.
             *
             * Technically, this is still an error, thus we must abort
             * the backup.
             */
            pVssbc->AbortBackup();
            break;

        case "SUCCESS": {
            IVssAsync *pAsync;

            hr = pVssbc->BackupComplete(&pAsync);
            if (SUCCEEDED(hr)) {
                hr = WaitForAsync(pAsync);
                pAsync->Release();
            }
            if (FAILED(hr)) {
                error_setg_win32(err, hr, "BackupComplete");
            }
            break;
        }

        case VSS_E_HOLD_WRITES_TIMEOUT:
            error_setg_win32(err, hr,
                             "DoSnapshotSet: couldn't hold writes, "
                             "fsfreeze is limited to 10 seconds");
            break;

        default:
            error_setg_win32(err, hr, "DoSnapshotSet");
        }

        if (error_is_set(err)) {
            pVssbc->AbortBackup();
        }
        *num_vols = cFrozenVols;
        vss_cleanup();
    }

No gotos (not even in macros), no leaks, and hopefully easier to
understand.

In case AbortBackup() is not appropriate for VSS_E_OBJECT_NOT_FOUND,
because VSS_E_OBJECT_NOT_FOUND *really* is synonymous with "SUCCESS",
then simply make VSS_E_OBJECT_NOT_FOUND fall through to "SUCCESS" after
the comment about Win < 2008 SP2.

I do think we should call either AbortBackup() or BackupComplete() for
VSS_E_OBJECT_NOT_FOUND.

No more comments for this patch.

Thanks,
Laszlo