[Box Backup-dev] [PATCH] Win32 merge [05] bbackupd

[Martin Ebourne]

On Sun, 2006-07-23 at 02:11 +0100, Chris Wilson wrote:
> The attached patch contains a few changes:
> 
> * Reformat code in bin/bbackupd/Win32ServiceFunctions.cpp to better match
>    the Box coding standards (not claiming it's perfect yet)
> * Allow passing a configuration file name to InstallService, and include
>    that name in the service command line
> * Pass the configuration file name from the command line when calling
>    OurService() and InstallService() from main()
> * Set a non-zero exit code if InstallService or RemoveService fails
> * Detect some common error cases in InstallService() and show better
>    debugging messages on the console

Why the chained elseifs? switch would be cleaner and less error prone.

> * Stop the Box Backup service before trying to delete it
> * Fixed a trivial memory leak reported by DebugMemleakFinder

There seems to be a change from "Win32BackupService gDaemonService" to
"Win32BackupService* gDaemonService", not sure which of the above that
relates to. Is it this memory leak fix?

I don't like this defining globals in a .cpp file and then declaring
them extern in another .cpp to get access. At the minimum the first .cpp
file's .h file should be declaring them. Encapsulation would be even
nicer if appropriate, I don't know anything about the windows API here
apart from what I can guess from the code, so not sure on this.

> * Removed WSAStartup() and WSACleanup() from bbackupd.cpp (moved to
>    Daemon.cpp) - temporarily breaks Win32 builds, pending the patch to
>    Daemon.cpp!

These should be part of the same changeset that adds the code back into
Daemon.cpp. Then there would be no breakage and everything would be
consistent. Prefer change based merge to file based merge. :)

Notwithstanding the above, the rest of it looks fine to me.

Cheers,

Martin.