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

[Ben Summers]

On 5 Dec 2005, at 17:17, Chris Wilson wrote:

> Hi Ben, Nick and all,
>
> On Mon, 5 Dec 2005, Ben Summers wrote:
>
>> I do not want to include a regex library in the sources. I would  
>> much prefer to not include it at all when it's not available, and  
>> make it easy to include it when it is.
>
> OK, fair enough. If it's OK with you, I will distribute a regex  
> library with Boxi, so that both Box and Boxi can use it when built  
> from that distribution.

That's not a problem.

>
>>> >  There are two parts to the Win32 port:
>>> > >  1) The actual porting effort.
>>> > >  2) Some extra features, for example, the dumping of state to  
>>> disc.
>>> > >  They should, perhaps be separated.
>
> Nick had to do a LOT of work to get Box working on win32 - the  
> POSIX emulation library he wrote is 1600 lines. I also think that  
> he did a great job, and I want to thank and congratulate him on that.

Yes, it's all very good. And quite something to emulate enough of POSIX!

>
> I also want to say, before I start nitpicking every line of  
> code :-) that none of what I write below is intended to criticise  
> that work. I just want to minimise the number of changes to Box in  
> order to make the merge as smooth as possible.
>
> On the question of what should be considered as strictly Win32  
> porting, and what constitutes extra features or bug fixes, I have a  
> few questions:
>
> 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.

>
> 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.

>
> 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.

>
> 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.


> 8. Nick's HousekeepStoreAccounts does some extra stuff when  
> housekeeping
>    is aborted:
>
>         if(!continueHousekeeping)
>         {
> +               // If any files were marked "delete now", then update
> +               // the size of the store.
> +               if(mBlocksUsedDelta != 0 || mBlocksInOldFilesDelta ! 
> = 0
> +                               || mBlocksInDeletedFilesDelta != 0)
> +               {
> +                       info->ChangeBlocksUsed(mBlocksUsedDelta);
> + info->ChangeBlocksInOldFiles(mBlocksInOldFilesDelta);
> + info->ChangeBlocksInDeletedFiles(mBlocksInDeletedFilesDe
> lta);
> +
> +                       // Save the store info back
> +                       info->Save();
> +               }
> +
>                 return;
>         }
>
>    Does this count as a bug fix for a separate patch?

I think I added that in 0.09. Nick did merge in some changes 0.08 ->  
0.09 manually.

>
> 9. Ditto for the check for a null buffer pointer before freeing it in
>    BackupStoreFileCombine.cpp
>
> 10. Someone else should review Nick's changes to  
> BackupStoreFile.cpp, as I
>     don't understand the alignment restrictions that his  
> CodingChunkAlloc
>     is enforcing, nor how the call to MaxBlockSizeForChunkSize can be
>     replaced with a fixed size of 256.

I will do that tomorrow.

>
> 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.

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

Yes.

>
> 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.

>
> 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.

>
> 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.

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

Ben