[v4,07/10] qemu-ga: Add Windows VSS requester to quiesce applications and filesystems

Add VSS requester functions for to qemu-ga.
This provides facility to request VSS service in Windows guest to quiesce
applications and filesystems. This function is only supported in Windows
2003 or later. In older guests, this function does nothing.

In several versions of Windows which don't support attribute
VSS_VOLSNAP_ATTR_NO_AUTORECOVERY, DoSnapshotSet fails with error
VSS_E_OBJECT_NOT_FOUND. In this patch, we just ignore this error.
To solve this fundamentally, we need a framework to handle mount writable
snapshot on guests, which is required by VSS auto-recovery feature
(a cleanup phase after snapshot is taken).

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/Makefile.objs           |    3 
 qga/vss-win32-requester.cpp |  419 +++++++++++++++++++++++++++++++++++++++++++
 qga/vss-win32-requester.h   |   31 +++
 3 files changed, 452 insertions(+), 1 deletion(-)
 create mode 100644 qga/vss-win32-requester.cpp
 create mode 100644 qga/vss-win32-requester.h

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   <tomoki.sekiyama@hds.com>
> + *
> + * 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   <tomoki.sekiyama@hds.com>
> + *
> + * 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

On 06/28/13 20:01, Laszlo Ersek wrote:

> (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__)

Typo of course, that's error_set_win32() in the replacement text. Sorry!

Laszlo

On 6/28/13 14:01 , "Laszlo Ersek" <lersek@redhat.com> wrote:
>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
...
>>+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.

I will make the return type gboolean, that represent if it is successful
or not. It is used only in main.c, as 'if (FAILED(vss_init())) { ...'.

>(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.)

Will use gboolean here as well.

>> +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?

Ok, I will also add a header for definition for "Error" (qapi/error.h).


>> 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
...
>> +#include <stdio.h>
>> +#include <assert.h>
>
>Can you remove this #include and replace all assert() occurrences with
>g_assert()?

Sure.

>> +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?

Oh, I didn't know that. Without underscores, they conflict with function
declared in the MinGW header. Maybe pCreateVssBackupComponents?

>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.

All right.

>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.)

OK. And making them a struct sounds like a good idea.

>> +
>> +GCC_FMT_ATTR(1, 2)
>> +static void errmsg(const char *fmt, ...)

I will remove this; fprintf(stderr,...) is good enough.

>> +
>> +static void error_set_win32(Error **errp, DWORD err,
>> +                            ErrorClass eclass, const char *text)
>> +{
...
>> +
>> +    /* 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.

g_win32_error_message is actually a wrapper for FormatMessage, and
FormatMessage actually can handle hresult (at least, in most cases).

>> +#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":
...
>(b) error_setg_win32() should imitate error_setg_errno():
...
>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()).

OK, I will take this way and split this into another patch.

>> +
>> +#define __chk(status, text, errp, err_label)    \
...
>> +#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.)

Okey. (Actually async/wait patterns spoil the benefit of chk()...)

>> +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.

Actually no, main purpose was to call COMRegister()/COMUnregister().

>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.)

Hmm, these file contains function commonly used both in POSIX systems
and win32. I will move VSSCheckOSVersion into vss-win32-requester.c,
because it is used only in the path from the file.

>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().

OK, I will add error messages.

>> +    hr = CoInitialize(NULL);
>> +    if (FAILED(hr)) {
>> +        errmsg("CoInitialize failed [%lx]", hr);
>> +        goto out;
>> +    };
>
>From what you said recently, CoInitialize() can't fail.

Will remove error check from here.

>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.)

It is only called on starting up qemu-ga.exe.
fprintf(stderr, ...) might be good enough here.
(I thought that a dialog might be good for qemu-ga as a system service,
 but it seems not appropriate.
 http://stackoverflow.com/questions/7735945/create-dialog-in-windows-services-at-vista-system )
 
>>+static void vss_cleanup(void)
...
>> +void vss_deinit(void)
...
>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.

Agreed. I will introduce a struct for static vars and make "vss_cleanup()"
clean up it.

>> +static void vss_add_components(Error **err)
...
>> +    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.)

Will remove chk()s and fix it.

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

Right... will free it in "out".

>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().

I believe so.

>> +    chk(_CreateVssBackupComponents(&pVssbc));
>> +    chk(pVssbc->InitializeForBackup());
>
>Hopefully AbortBackup() is valid for "pVssbc" even when
>InitializeForBackup() fails first.

It just returns error in the worst case.

>> +    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.

Right. Will fix them (and remove chk()s).

>> +    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.

OK. And "h == INVALID_HANDLE_VALUE" should be treated as an error.

>> +        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())

I confirmed that it works.

>> +                error_setg_win32(err, hr, msg);
>> +                goto out;
>
>You miss FindVolumeClose() here.

Oops...

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

Sure.

>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.

Yeah... I will add "goto out;" for that case.

>> +        error_setg_win32(err, GetLastError(), "CreateEvenet");
>typo in "CreateEvenet"
>> +        error_setg_win32(err, GetLastError(), "CreateEvenet");
>The typo was replicated here.

Oops, will fix it.

>> +    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.

Exactly.

>> +    for (int i = 0; i < 1000; i++) {
>What do you think of a macro for the repeat count?

Sure.

>> +        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.

Maybe we don't need it. (though I remember some sample code was doing this...)

>> +        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.

Agreed. This should be "wait_status != WAIT_TIMEOUT" and
should check if "ret == WAIT_OBJECT_0" after the loop, as you said.

>> +
>> +    *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.

Right.
(However, actually VSS will keep the system frozen only for 10 seconds.
 If we exceeds that 10 seconds timeout, it is reported in the thaw event. )

>> +    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?

Ah, yes, it is already thawed at this point. What I meant here was,
"the systems had been frozen *until the guest-fsfreeze-thaw was issued*."

>> +        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).

Agreed.

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

It looks already aborted when we get VSS_E_OBJECT_NOT_FOUND.
AbortBackup just returns an error code.
However, calling AbortBackup just in case will not hurt anything.

>> +out:
>> +    if (pVssbc) {
>
>"pVssbc" is never NULL here (see the assert, for example).

Right. Will remove the check.

>> +        pVssbc->AbortBackup();
>> +    }
>> +done:
>> +    vss_cleanup();
>> +}
>
>For my taste, there are way too many complex goto's here. What do you
>think of the following:
...
>No gotos (not even in macros), no leaks, and hopefully easier to
>understand.

Thank you for the code. Looks really nice.

>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

Thanks a lot!

Tomoki Sekiyama

On 06/30/13 03:21, Tomoki Sekiyama wrote:
> On 6/28/13 14:01 , "Laszlo Ersek" <lersek@redhat.com> wrote:
>> On 06/06/13 17:06, Tomoki Sekiyama wrote:

>>> +static t_CreateVssBackupComponents _CreateVssBackupComponents;
>>> +static t_VssFreeSnapshotProperties _VssFreeSnapshotProperties;

>> Unless there's a pressing reason, could you drop the leading
>> underscores?
> 
> Oh, I didn't know that. Without underscores, they conflict with function
> declared in the MinGW header. Maybe pCreateVssBackupComponents?

Sure.

I hope to start reviewing the next patch soonish.

Thanks!
Laszlo