[Box Backup-dev] Re: [Box Backup] Win32 port

[Chris Wilson]

Hi Ben and all,

On Mon, 5 Dec 2005, Ben Summers wrote:

>>  1. Nick's bin/bbackupd/BackupClientContext.cpp changes the way that
>>    BackupClientContext::FindFilename works, by decrypting the filenames
>>    first rather than comparing encrypted names. The intent seems similar
>>     to that documented in BackupClientDirectoryRecord.cpp.
>> 
>>    This looks like a bug fix rather than Win32 port. Should I break it out
>>     into a separate patch?
>
> I suspect it would be better to somehow avoid this need in the first place. 
> But for now, that's probably fine.

OK, done. Removed in 
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/4-removed-decrypt-filenames/]

>>  2. Nick's BackupClientContext.cpp also includes a bunch of stuff related
>>     to maximum diffing time and SSL keepalives
>>     (BackupClientContext::SetMaximumDiffingTime). Is this already 
>>  included
>>    in someone else's patches (perhaps Gary's?), and should I consider it
>>    part of the Win32 port or break it out? It's also only enabled on Win32
>>    - if this is important then perhaps we should make it cross-platform?
>
> Cross platform is preferred.

Since this is covered by Gary's patch, I'll remove it for now. It 
contained some Win32-specific parts which Gary's patch doesn't have, so 
I'll send those to Gary for inclusion in his patch, when the time comes.
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/3-removed-garys-patch/]

>>  4. In BackupClientDirectoryRecord.cpp, Nick comments that lstat() failure
>>     at this point causes an exception which aborts the backup. This can
>>     happen if the file is locked (on win32) or removed during the 
>>  backup.
>>     Should I merge his comments to that effect, or try to find a better
>>     solution to this problem?
>
> If the contributors are willing to participate in a bit of a redesign of some 
> of the backup engine (to take advantage of stuff learnt from this version) 
> then merge the comments. Otherwise we should fix it.

I'm certainly willing to help with this effort in any way that I can, with 
my limited skills and knowledge of the code. It seems easier to merge with 
these comments than to try to redesign at this stage, so I vote that we 
include the comments, finish the merge, and then look at this problem 
again.

>>  6. Nick's BackupDaemon.cpp has a separate Windows thread for monitoring
>>    the named pipe for bbackupd control. Is this strictly required, or an
>>    improvement? My own patch set (for Boxi) does this in a different way -
>>     no extra thread is created, but instead the main thread polls the
>>     command socket more often. Any suggestions on which approach is
>>     preferred? The thread stuff looks harder to make cross-platform
>>     compatible.
>
> It's usually best to wait using the OS' provided API, rather than to poll.

Any suggestions on how to use threads in a cross-platform way to achieve 
this? I know wxWidgets has a cross-platform thread library, but it may be 
a bit too heavyweight a dependency for your taste.

>>  8. Nick's HousekeepStoreAccounts does some extra stuff when housekeeping
>>     is aborted:
[...]
> I think I added that in 0.09. Nick did merge in some changes 0.08 -> 0.09 
> manually.

You're right, this is already included in 0.09. In fact, the files are 
identical. I've removed this patch since it doesn't need to be merged.
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/5-removed-housekeeping-update-from-0.09]

>>  11. What's the rationale for private distributions? (BOX_PRIVATE_BEGIN)
>>      i.e. what code should be protected by such sections?
>
> It's some other stuff I wrote. I didn't write a generic UNIX daemon and 
> client set of libraries just to use it on one project. It should be pulled 
> out of the distribution.

Should I assume that nobody except you should be creating BOX_PRIVATE 
sections? (not that it matters now, there aren't any left in the win32 
port).

>>  12. I think that Nick's WinNamedPipe and PipeGetLine can, and should, be
>>      merged into SocketStream and IOStreamGetLine, reducing code
>>      duplication.
>
> Yes.

OK, I will have a deeper look at this.

>>  13. Nick's bbackupctl sends error messages to syslog (or the Windows Event
>>     Log) in addition to the standard output. I think this is not a good
>>     idea for a Unix command-line tool, and I'm not sure of the value on
>>     Windows. So I suggest either removing these entirely, or making them
>>      conditional on WIN32.
>
> I'd vote for remove (or perhaps #ifndef NDEBUG on WIN32 only). Maybe it was a 
> debugging thing? It can be tricky to see the output in syslog sometimes.

OK, done. 
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/6-made-bbackupctl-syslogging-conditional]

>>  14. The emulation library provides stubs for lstat and readlink which do
>>     nothing, since Windows doesn't have symbolic links. I suggest that we
>>      remove these and disable any tests or code that requires them on
>>      Win32. Otherwise they may present a hazard for the unwary coder in
>>      future.
>
> I would prefer that they threw an exception so that #ifdefs are minimised. 
> Although the tests can't get much more ugly.

If they throw exceptions, then the tests will have to do something like 
this:

 	#ifdef WIN32
 	TEST_CHECK_THROWS(<readlink call>, SomeException, SubType)
 	#else
 	TEST_THAT(<readlink_call> == 0)
 	#endif

Is that better than just #ifndef WIN32 around the call? If so then I'm 
happy to implement it, if you can tell me what kind of exception you'd 
like it to throw. Although I personally prefer that such code is checked 
at compile time rather than runtime.

>>  15. Nick's Configuration.cpp contains code to detect when the config file
>>     has been modified and reload it automatically. Shall I break this out
>>      into a separate patch?
>
> I'd prefer it if that never made it into the tree. It'll break some of my 
> code, and is a bit nasty anyway.

It seems to have been part of Gary's 0.09-mod patch, so it's gone now that 
that patch is removed.

> Again, take comments as wanting the next Box Backup to be as high quality as 
> possible, rather than criticising the code randomly. (Thanks Nick)

Nick++ :-)

I'm eagerly awaiting a barrage of criticism for my own misdeeds in the 
name of the porting effort. :-)

Cheers, Chris.
-- 
_ ___ __     _
  / __/ / ,__(_)_  | Chris Wilson <0000 at qwirx.com> - Cambs UK |
/ (_/ ,\/ _/ /_ \ | Security/C/C++/Java/Perl/SQL/HTML Developer |
\ _/_/_/_//_/___/ | We are GNU-free your mind-and your software |