From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UscxY-0005gH-Dp for qemu-devel@nongnu.org; Fri, 28 Jun 2013 14:00:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UscxN-0006pg-Ff for qemu-devel@nongnu.org; Fri, 28 Jun 2013 13:59:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33258) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UscxN-0006oc-1m for qemu-devel@nongnu.org; Fri, 28 Jun 2013 13:59:37 -0400 Message-ID: <51CDCF8A.4080005@redhat.com> Date: Fri, 28 Jun 2013 20:01:46 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <20130606150618.10486.60669.stgit@hds.com> <20130606150649.10486.6597.stgit@hds.com> In-Reply-To: <20130606150649.10486.6597.stgit@hds.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 07/10] qemu-ga: Add Windows VSS requester to quiesce applications and filesystems List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tomoki Sekiyama Cc: libaiqing@huawei.com, mdroth@linux.vnet.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, vrozenfe@redhat.com, pbonzini@redhat.com, seiji.aguchi@hds.com, areis@redhat.com 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 > + * > + * 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 > + * > + * 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 > +#include 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