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

[Chris Wilson]

Hi Ben, Nick and all,

On Mon, 5 Dec 2005, Ben Summers wrote:

> I think we should go with UNIX style endings whereever it does not cause 
> problems with Win32. Otherwise SVN doesn't work terribly well with diffs.

Aye aye.

>>  I would like to clean up the comments and line lengths before this gets 
>>  merged, since I'm certain you won't allow me to change them afterwards :-) 
>>  Any objections?
>
> Cleaning up sounds good to me!

You can find my first attempt at 
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/2-code-formatting/]. I 
haven't tested whether it compiles yet, but you can compare it with 
[http://bbdev.fluffy.co.uk/svn/box/chris/win32/1-line-endings/] for 
reference. Comments welcome.

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

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

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?

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?

3. Nick created a new member in BackupClientContext.h, a boolean called
    mbIsManaged. Since we're not using full Hungarian notation (thank
    ****), perhaps it should be called mIsManaged?

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?

5. Nick made a number of things conditional on BERKELY_V4, for example:

--- bb-0.8/bin/bbackupd/BackupClientInodeToIDMap.cpp
+++ bb-win32-nick/bin/bbackupd/BackupClientInodeToIDMap.cpp
@@ -69,7 +71,11 @@
  #ifndef BACKIPCLIENTINODETOIDMAP_IN_MEMORY_IMPLEMENTATION
         if(dbp != 0)
         {
+#ifdef BERKELY_V4
+               dbp->close(0);
+#elif
                 dbp->close(dbp);
+#endif
         }
  #endif
  }

    I'm no bdb guru, but is db4 not backwards-compatible with previous
    versions? Shouldn't it be possible to write the code so that it works
    with any version of bdb?

    In any case, BERKELY_V4 isn't defined in BoxPlatform.h (it's commented
    out), so can we just remove all of these for now?

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.

7. Nick's BackupDaemon.cpp has the following code:

+       DWORD timeout = BoxTimeToMilliSeconds(RequiredDelay);
+
+       while ( this->mReceivedCommandConn == false )
+       {
+               Sleep(1);
+
+               if ( timeout == 0 )
+               {
+                       DoSyncFlagOut = false;
+                       SyncIsForcedOut = false;
+                       return;
+               }
+               timeout--;
+       }

    This looks wrong to me. We sleep for one second, but decrease the
    timeout (in milliseconds) by one?

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?

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.

11. What's the rationale for private distributions? (BOX_PRIVATE_BEGIN)
     i.e. what code should be protected by such sections?

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

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.

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.

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?

</nitpick>

All comments gratefully appreciated.

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 |