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

[Chris Wilson]

Hi all,

On Sat, 28 Jan 2006, Ben Summers wrote:

>>   svn diff -r 336:340 http://bbdev.fluffy.co.uk/svn/box/chris/bb-save-state
>
> 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.

No, it is used. Not to reload the config, but to check that the config
has not changed since the archived state was written. If it has changed,
the archive should be discarded.

> bin/bbackupd/BackupDaemon.cpp
> 	-- how long does storing the config take? Might it be better to do it 
> when the daemon terminates only?

I'd say it's better to do at the end of each run, when we're not in a 
hurry. During system shutdown, daemons have only a limited and 
system-dependent amount of time to clean up before being killed by the 
system.

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

OK, done.

By the way, Gary, what's with all the comments like this:

         //
         //
         //

What purpose does that serve? It's not at all obvious to me, and it should 
be.

> lib/common/Archive.h
> 	-- Is CommonException/StreamableMemBlockIncompleteRead the right 
> exception to throw?

Probably not. It looks like Archive.h was based on StreamableMemBlock.h. 
Would you prefer OSFileReadError, or should I create a new exception like 
OSFileShortRead, or FileEndedUnexpectedly?

> +               int privItem;
> +               Get(privItem);
> +
> +               if (privItem)
> +                       bItem = true;
> +               else
> +                       bItem = false;
> 	-- I prefer braces on ALL blocks. Call me paranoid and all that...

Done, but that was the only instance I could find. If there are others, 
please let me know.

> I see a lot of similarities between Archive and Protocol. Using Protocol with 
> auto-generated definitions would catch coding errors later.

Not sure that I'm up to the task of converting it. Perhaps Gary can help?

> 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"?

Done.

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

Absolutely.

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

How about BUGS.txt?

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

Presumably refactoring it to derive from Protocol would fix that?

> I have not run the tests yet.

All existing tests pass on i386 FC2.

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 |