From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ut6Ki-000688-5C for qemu-devel@nongnu.org; Sat, 29 Jun 2013 21:21:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ut6Ke-0007hJ-6R for qemu-devel@nongnu.org; Sat, 29 Jun 2013 21:21:40 -0400 Received: from usindpps04.hds.com ([207.126.252.17]:34868) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ut6Kd-0007gl-SZ for qemu-devel@nongnu.org; Sat, 29 Jun 2013 21:21:36 -0400 From: Tomoki Sekiyama Date: Sun, 30 Jun 2013 01:21:23 +0000 Message-ID: In-Reply-To: <51CDCF8A.4080005@redhat.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Laszlo Ersek 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 , "areis@redhat.com" On 6/28/13 14:01 , "Laszlo Ersek" wrote:=0A= >On 06/06/13 17:06, Tomoki Sekiyama wrote:=0A= >=0A= >> diff --git a/qga/vss-win32-requester.h b/qga/vss-win32-requester.h=0A= >> new file mode 100644=0A= >> index 0000000..f180f56=0A= >> --- /dev/null=0A= >> +++ b/qga/vss-win32-requester.h=0A= ...=0A= >>+HRESULT vss_init(void);=0A= >=0A= >Can you include the mingw header that defines the HRESULT type? As far=0A= >as I know we like to make headers standalone.=0A= =0A= I will make the return type gboolean, that represent if it is successful=0A= or not. It is used only in main.c, as 'if (FAILED(vss_init())) { ...'.=0A= =0A= >(OTOH I vaguely recall a patch where the order between a mingw header=0A= >and a (generated?) trace header could cause a build error... I think it=0A= >would be worth trying still.)=0A= >=0A= >> +void vss_deinit(void);=0A= >> +int vss_initialized(void);=0A= >=0A= >Since this is qemu-ga / qemu code, I think a "bool" return type would be= =0A= >more usual. (The current prototype is correct too of course.)=0A= =0A= Will use gboolean here as well.=0A= =0A= >> +void qga_vss_fsfreeze_freeze(int *nr_volume, struct Error **err);=0A= >> +void qga_vss_fsfreeze_thaw(int *nr_volume, struct Error **err);=0A= >=0A= >Can you drop the "struct" word in these prototypes?=0A= =0A= Ok, I will also add a header for definition for "Error" (qapi/error.h).=0A= =0A= =0A= >> diff --git a/qga/vss-win32-requester.cpp b/qga/vss-win32-requester.cpp= =0A= >> new file mode 100644=0A= >> index 0000000..7784926=0A= >> --- /dev/null=0A= >> +++ b/qga/vss-win32-requester.cpp=0A= ...=0A= >> +#include =0A= >> +#include =0A= >=0A= >Can you remove this #include and replace all assert() occurrences with=0A= >g_assert()?=0A= =0A= Sure.=0A= =0A= >> +static t_CreateVssBackupComponents _CreateVssBackupComponents;=0A= >> +static t_VssFreeSnapshotProperties _VssFreeSnapshotProperties;=0A= >=0A= >I apologize in advance for splitting hairs, but :)=0A= >=0A= > 17.4.3.1.2 Global names [lib.global.names]; p1=0A= >=0A= > Certain sets of names and function signatures are always reserved to=0A= > the implementation:=0A= >=0A= > - Each name that contains a double underscore (__) or begins with an=0A= > underscore followed by an uppercase letter (2.11) is reserved to the= =0A= > implementation for any use.=0A= >=0A= >Unless there's a pressing reason, could you drop the leading=0A= >underscores?=0A= =0A= Oh, I didn't know that. Without underscores, they conflict with function=0A= declared in the MinGW header. Maybe pCreateVssBackupComponents?=0A= =0A= >Can you decorate each of these static variables with a short comment? It= =0A= >does get clear(er) what they are used for by reading the code, but the=0A= >comments would save others time.=0A= =0A= All right.=0A= =0A= >Also I'd recommend grouping them differently:=0A= >=0A= >- first group: long term objects related to VSSAPI.DLL and released=0A= > *only* in vss_deinit(): hLib, _CreateVssBackupComponents,=0A= > _VssFreeSnapshotProperties;=0A= >=0A= >- second group: objects that make sense only in preparation for, during,= =0A= > and right after a freeze: pVssbc, pAsyncSnapshot, hEvent, hEvent2,=0A= > cFrozenVols. (You could even introduce a struct for these, but that's=0A= > just an idea.)=0A= =0A= OK. And making them a struct sounds like a good idea.=0A= =0A= >> +=0A= >> +GCC_FMT_ATTR(1, 2)=0A= >> +static void errmsg(const char *fmt, ...)=0A= =0A= I will remove this; fprintf(stderr,...) is good enough.=0A= =0A= >> +=0A= >> +static void error_set_win32(Error **errp, DWORD err,=0A= >> + ErrorClass eclass, const char *text)=0A= >> +{=0A= ...=0A= >> +=0A= >> + /* set error message in UTF-8 encoding */=0A= >> + msg =3D g_win32_error_message(err);=0A= >=0A= >Can we pass "err" (which is a HRESULT =3D=3D=3D DWORD =3D=3D=3D "long unsi= gned") to=0A= >g_win32_error_message()? The latter takes an "int".=0A= > =0A= >MSDN seems to imply there are different "error code spaces", see=0A= >HRESULT_FROM_WIN32(). Anyway I'll assume we can do this.=0A= =0A= g_win32_error_message is actually a wrapper for FormatMessage, and=0A= FormatMessage actually can handle hresult (at least, in most cases).=0A= =0A= >> +#define error_setg_win32(errp, err, text) \=0A= >> + error_set_win32(errp, err, ERROR_CLASS_GENERIC_ERROR, text)=0A= >=0A= >This approach is fine in general for the VSS DLL I think, but for=0A= >qemu-ga.exe, I would propose a more flexible interface (could even=0A= >belong into "util/error.c", in an #ifdef _WIN32 block). It would imitate= =0A= >error_setg_errno() in that it allowed the caller to pass in a format=0A= >string as well:=0A= >=0A= >(a) Create a copy of error_set_errno() with "errno"/"strerror" replaced=0A= >by "win32"/"g_win32_error_message":=0A= ...=0A= >(b) error_setg_win32() should imitate error_setg_errno():=0A= ...=0A= >This is just a suggestion, but it would bring your code closer to "qemu=0A= >coding style", and render free-form error messages more convenient for=0A= >you (eg. you could drop the snprintf() stuff in=0A= >qga_vss_fsfreeze_freeze()).=0A= =0A= OK, I will take this way and split this into another patch.=0A= =0A= >> +=0A= >> +#define __chk(status, text, errp, err_label) \=0A= ...=0A= >> +#define _chk(status, msg) __chk(status, "Failed to " msg, err, out)=0A= >> +#define chk(status) __chk(status, "Failed to " #status, err, out)=0A= >=0A= >I'd prefer if you hand-expanded these macros in qemu-ga code. The=0A= >error_setg_win32() version suggested above should make it easy to format= =0A= >any error message. You'd have to open-code the "goto" and the specific=0A= >label naturally, but that would also match the rest of qemu code better=0A= >IMHO. (More work to write once, but easier to read many times.)=0A= =0A= Okey. (Actually async/wait patterns spoil the benefit of chk()...)=0A= =0A= >> +HRESULT vss_init(void)=0A= >> +{=0A= >> + HRESULT hr;=0A= >> +=0A= >> + hr =3D VSSCheckOSVersion();=0A= >> + if (hr =3D=3D S_FALSE) {=0A= >> + return hr;=0A= >> + }=0A= >=0A= >Ah, I think you may have addressed this already -- this is the reason=0A= >"vss-win32-provider/install.o" is needed when linking qemu-ga.exe.=0A= =0A= Actually no, main purpose was to call COMRegister()/COMUnregister().=0A= =0A= >VSSCheckOSVersion() is quite small, relies on no C++ *or* VSS features,=0A= >and is arguably a utility function. Can you move it to one of:=0A= >- util/osdep.c,=0A= >- os-win32.c,=0A= >- util/oslib-win32.c?=0A= >=0A= >(I'm not sure exactly which one would be best, but I guess Paolo can=0A= >tell.)=0A= =0A= Hmm, these file contains function commonly used both in POSIX systems=0A= and win32. I will move VSSCheckOSVersion into vss-win32-requester.c,=0A= because it is used only in the path from the file.=0A= =0A= >Furthermore, if VSSCheckOSVersion() fails, we should notify the user=0A= >(based at least on the fact that other errors in vss_init() get=0A= >reported). In "install.cpp" too you report VSSCheckOSVersion() failures=0A= >with printf().=0A= =0A= OK, I will add error messages.=0A= =0A= >> + hr =3D CoInitialize(NULL);=0A= >> + if (FAILED(hr)) {=0A= >> + errmsg("CoInitialize failed [%lx]", hr);=0A= >> + goto out;=0A= >> + };=0A= >=0A= >>From what you said recently, CoInitialize() can't fail.=0A= =0A= Will remove error check from here.=0A= =0A= >Independently, is errmsg() -- or as I'm proposing, errmsg_dialog() --=0A= >justified here? (I don't know about the context(s) yet you're going to=0A= >call vss_init() from.)=0A= =0A= It is only called on starting up qemu-ga.exe.=0A= fprintf(stderr, ...) might be good enough here.=0A= (I thought that a dialog might be good for qemu-ga as a system service,=0A= but it seems not appropriate.=0A= http://stackoverflow.com/questions/7735945/create-dialog-in-windows-servic= es-at-vista-system )=0A= =0A= >>+static void vss_cleanup(void)=0A= ...=0A= >> +void vss_deinit(void)=0A= ...=0A= >This separation of cleanup tasks seems apt (peeking forward a bit).=0A= >=0A= >However, from all the static(-ally referenced) data, "pAsyncSnapshot" is= =0A= >not handled in vss_cleanup(). Peeking forward a bit again, I think that=0A= >"pAsyncSnapshot" might be leaked if the loop in=0A= >qga_vss_fsfreeze_freeze() terminates with a QueryStatus() error.=0A= =0A= Agreed. I will introduce a struct for static vars and make "vss_cleanup()"= =0A= clean up it.=0A= =0A= >> +static void vss_add_components(Error **err)=0A= ...=0A= >> + chk(pVssbc->GetWriterMetadataCount(&cWriters));=0A= >=0A= >This could jump to "out" with an indeterminate "pComponent" (and call=0A= >pComponent->Release()).=0A= >=0A= >I think the chk() macro makes a false promise -- you can't avoid=0A= >examining error conditions individually. (Eliminating chk() & co., as=0A= >I've suggested, should help with this too.)=0A= =0A= Will remove chk()s and fix it.=0A= =0A= >Should we free "bstrWriterName" too? (If so, then we must check it under= =0A= >"out" as well I guess.)=0A= =0A= Right... will free it in "out".=0A= =0A= >Presumably, if this function (or something after it) fails, any=0A= >components added with pVssbc->AddComponent() will be removed by=0A= >pVssbc->AbortBackup() or pVssbc->Release() (in vss_cleanup()) on the=0A= >error path in qga_vss_fsfreeze_freeze().=0A= =0A= I believe so.=0A= =0A= >> + chk(_CreateVssBackupComponents(&pVssbc));=0A= >> + chk(pVssbc->InitializeForBackup());=0A= >=0A= >Hopefully AbortBackup() is valid for "pVssbc" even when=0A= >InitializeForBackup() fails first.=0A= =0A= It just returns error in the worst case.=0A= =0A= >> + chk(pVssbc->GatherWriterMetadata(&pAsync));=0A= >> + _chk(WaitForAsync(pAsync), "GatherWriterMetadata");=0A= >=0A= >If WaitForAsync() fails, this leaks pAsync.=0A= >=0A= >> + pAsync->Release();=0A= >=0A= >OTOH once you start handling pAsync under "out", you'll need to null it=0A= >here.=0A= =0A= Right. Will fix them (and remove chk()s).=0A= =0A= >> + h =3D FindFirstVolumeW(buf, sizeof(buf));=0A= >> + while (h !=3D INVALID_HANDLE_VALUE) {=0A= >=0A= >"h" doesn't appear to change in the loop. Hence I'd prefer an "if" after= =0A= >FindFirstVolumeW(), and a for (;;) here.=0A= =0A= OK. And "h =3D=3D INVALID_HANDLE_VALUE" should be treated as an error.=0A= =0A= >> + if (GetDriveTypeW(buf) =3D=3D DRIVE_FIXED) {=0A= >> + VSS_ID pid;=0A= >> + hr =3D pVssbc->AddToSnapshotSet(buf, g_gProviderId, &pid);= =0A= >> + if (FAILED(hr)) {=0A= >> + WCHAR name[PATH_MAX];=0A= >> + char msg[PATH_MAX+32];=0A= >> + if (GetVolumePathNamesForVolumeNameW(=0A= >> + buf, name, sizeof(name), NULL) && *name) {=0A= >> + b =3D name;=0A= >> + }=0A= >> + snprintf(msg, sizeof(msg), "add %S to snapshot set",=0A= >>b);=0A= >=0A= >(let's hope %S won't run into EILSEQ -- the same applies if you agree to= =0A= >rebase this code to the proposed error_setg_win32())=0A= =0A= I confirmed that it works.=0A= =0A= >> + error_setg_win32(err, hr, msg);=0A= >> + goto out;=0A= >=0A= >You miss FindVolumeClose() here.=0A= =0A= Oops...=0A= =0A= >> + }=0A= >> + n++;=0A= >=0A= >Can you rename=0A= >- "n" to "num_fixed_drives",=0A= >- "h" to "volume",=0A= >- "buf" to "short_volume_name",=0A= >- "name" to "volume_path_names"?=0A= =0A= Sure.=0A= =0A= >Just a suggestion: if "n" ("num_fixed_drives") is zero here, we could=0A= >short-circuit the fsfreeze command (no volume added to the snapshot=0A= >set). OTOH I can't imagine any windows installation without a fixed=0A= >drive, admittedly.=0A= =0A= Yeah... I will add "goto out;" for that case.=0A= =0A= >> + error_setg_win32(err, GetLastError(), "CreateEvenet");=0A= >typo in "CreateEvenet"=0A= >> + error_setg_win32(err, GetLastError(), "CreateEvenet");=0A= >The typo was replicated here.=0A= =0A= Oops, will fix it.=0A= =0A= >> + chk(pVssbc->DoSnapshotSet(&pAsyncSnapshot));=0A= >=0A= >... The events are closed in vss_cleanup() on failure, OK.=0A= >=0A= >Am I right to think that this is the point where VSS enters=0A= >CQGAVssProvider::CommitSnapshots()? In that case the ordering of the two= =0A= >CreateEvent() calls vs. DoSnapshotSet() is correct, the OpenEvent()=0A= >calls in CQGAVssProvider::CommitSnapshots() will find the events.=0A= =0A= Exactly.=0A= =0A= >> + for (int i =3D 0; i < 1000; i++) {=0A= >What do you think of a macro for the repeat count?=0A= =0A= Sure.=0A= =0A= >> + HRESULT hr =3D S_OK;=0A= >=0A= >Is the initialization necessary? If QueryStatus() succeeds, "hr" should=0A= >be overwritten, should it not? Just asking because the initialization=0A= >implies we might read "hr" without any further assignment to it.=0A= =0A= Maybe we don't need it. (though I remember some sample code was doing this.= ..)=0A= =0A= >> + DWORD ret =3D WaitForSingleObject(hEvent, 10);=0A= >> + if (ret =3D=3D WAIT_OBJECT_0) {=0A= >> + break;=0A= >> + }=0A= >=0A= >Should we look for other possible return values? For example,=0A= >WAIT_FAILED would render the rest of the loop moot.=0A= =0A= Agreed. This should be "wait_status !=3D WAIT_TIMEOUT" and=0A= should check if "ret =3D=3D WAIT_OBJECT_0" after the loop, as you said.=0A= =0A= >> +=0A= >> + *num_vols =3D cFrozenVols =3D n;=0A= >> + return;=0A= >=0A= >Right. At this point we return to libvirt, from the fsfreeze command,=0A= >the file systems are frozen, and CQGAVssProvider::CommitSnapshots() is=0A= >blocking, for 60 seconds, on the "thaw" event.=0A= =0A= Right.=0A= (However, actually VSS will keep the system frozen only for 10 seconds.=0A= If we exceeds that 10 seconds timeout, it is reported in the thaw event. )= =0A= =0A= >> + HRESULT hr =3D WaitForAsync(pAsyncSnapshot);=0A= >> + if (hr =3D=3D (HRESULT)VSS_E_OBJECT_NOT_FOUND) {=0A= >> + /*=0A= >> + * On Windows earlier than 2008 SP2 which does not support=0A= >> + * VSS_VOLSNAP_ATTR_NO_AUTORECOVERY context, the final commit= =0A= >>is not=0A= >> + * skipped and VSS is aborted by VSS_E_OBJECT_NOT_FOUND.=0A= >>Still, as the=0A= >> + * applications and file systems are frozen, we just ignore=0A= >>the error.=0A= >=0A= >I think the comment is mis-worded, applications and file systems should=0A= >be thawed, not frozen, at this point, no?=0A= =0A= Ah, yes, it is already thawed at this point. What I meant here was,=0A= "the systems had been frozen *until the guest-fsfreeze-thaw was issued*."= =0A= =0A= >> + pAsyncSnapshot->Release();=0A= >> + pAsyncSnapshot =3D NULL;=0A= >=0A= >As I've been parroting above :), these two steps should happen in=0A= >vss_cleanup() -- conditionally of course, like the rest there.=0A= >=0A= >You could also move "cFrozenVols =3D 0" to that function (see the grouping= =0A= >I recommended at the top).=0A= =0A= Agreed.=0A= =0A= >> + goto final;=0A= >=0A= >Technically, the backup finished with an error. Should you not abort it,= =0A= >like you do in case of other errors?=0A= =0A= It looks already aborted when we get VSS_E_OBJECT_NOT_FOUND.=0A= AbortBackup just returns an error code.=0A= However, calling AbortBackup just in case will not hurt anything.=0A= =0A= >> +out:=0A= >> + if (pVssbc) {=0A= >=0A= >"pVssbc" is never NULL here (see the assert, for example).=0A= =0A= Right. Will remove the check.=0A= =0A= >> + pVssbc->AbortBackup();=0A= >> + }=0A= >> +done:=0A= >> + vss_cleanup();=0A= >> +}=0A= >=0A= >For my taste, there are way too many complex goto's here. What do you=0A= >think of the following:=0A= ...=0A= >No gotos (not even in macros), no leaks, and hopefully easier to=0A= >understand.=0A= =0A= Thank you for the code. Looks really nice.=0A= =0A= >In case AbortBackup() is not appropriate for VSS_E_OBJECT_NOT_FOUND,=0A= >because VSS_E_OBJECT_NOT_FOUND *really* is synonymous with "SUCCESS",=0A= >then simply make VSS_E_OBJECT_NOT_FOUND fall through to "SUCCESS" after=0A= >the comment about Win < 2008 SP2.=0A= >=0A= >I do think we should call either AbortBackup() or BackupComplete() for=0A= >VSS_E_OBJECT_NOT_FOUND.=0A= >=0A= >No more comments for this patch.=0A= >=0A= >Thanks,=0A= >Laszlo=0A= =0A= Thanks a lot!=0A= =0A= Tomoki Sekiyama=