[Box Backup-dev] Re: First official Win32 release

[Ben Summers]

On 27 Jan 2006, at 23:32, Chris Wilson wrote:

> Hi Gary,
>
> On Fri, 27 Jan 2006, Gary wrote to boxbackup@fluffy.co.uk:
>
>>> I can create another branch with the save state patch applied,  
>>> for inspection and hopefully merging if you like.
>>
>> Plase do, I will be able to head and change the non-standard  
>> operator stuff to match the rest of the sources.
>
> Done. Please inspect and test the patch:
>
>   svn diff -r 336:340 http://bbdev.fluffy.co.uk/svn/box/chris/bb- 
> save-state
>
> All tests pass on my i386 FC2 box.

lib/common/Configuration.h
lib/common/Configuration.cpp
	-- is this really necessary? Not sure this functionality should be  
in the Configuration class, and I haven't noticed this used anywhere.  
Please remove.
	(I know this is used for automatically reloading the config when it  
changes. I don't like that feature at all.)

bin/bbackupd/BackupDaemon.cpp
	-- how long does storing the config take? Might it be better to do  
it when the daemon terminates only?
	-- You need to delete the stored file as soon as it's been read in.  
Consider what will happen if the process aborts, and is restarted.

lib/common/Archive.h
	-- Is CommonException/StreamableMemBlockIncompleteRead the right  
exception to throw?
+               int privItem;
+               Get(privItem);
+
+               if (privItem)
+                       bItem = true;
+               else
+                       bItem = false;
	-- I prefer braces on ALL blocks. Call me paranoid and all that...


I see a lot of similarities between Archive and Protocol. Using  
Protocol with auto-generated definitions would catch coding errors  
later. Just an observation, after all, this is tested (in the field,  
not unit tested) code which works.

bin/bbackupd/bbackupd-config needs to have a comment added in the  
generated config file to mention this feature. Unless there is a  
decent test it should not be enabled by default, and perhaps marked  
as "experimental"? (Given that this is a backup utility, I feel we  
would be rather irresponsible if we were not totally conservative  
about our approach to new stuff.)

We need to have a TODO file which lists all the stuff which we need  
to get done, eg tests for this.

I will not insist that operator overloaded i/o is changed (but feel  
free to do so). After climbing down on this issue wrt the logging i/ 
o, it would seem inconsistent to be stubborn here.

I have not run the tests yet.

Thanks for the code.

Ben