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

[Tomoki Sekiyama]

On 6/28/13 14:01 , "Laszlo Ersek" <address@hidden> 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