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

Ben Summers boxbackup-dev at fluffy.co.uk
Mon Dec 5 18:10:15 GMT 2005


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







More information about the Boxbackup-dev mailing list