[Box Backup-dev] Merges

Chris Wilson boxbackup-dev at fluffy.co.uk
Sun Aug 20 01:47:56 BST 2006


Hi Martin,

> Except where noted, these are all good to merge:

Thanks! I think I've merged all the ones you said were OK.

> --- bin/bbackupd/BackupDaemon.cpp       (revision 748)
> +++ bin/bbackupd/BackupDaemon.cpp       (revision 749)
> @@ -263,16 +263,36 @@
> void BackupDaemon::RunHelperThread(void)
> {
>        mpCommandSocketInfo = new CommandSocketInfo;
> -       this->mReceivedCommandConn = false;
>
> Why?

Sorry, looks like a mistake.

> 2. If you've got a catch(...) and you're not propagating the error you 
>    should at least catch(std::exception&) before it and report that with 
>    .what(). This will catch a lot of exceptions (eg. out of memory) 
>    that would otherwise go reported as "unknown error". I see too much 
>    software give unknown error and it's quite hateful. This is also true 
>    of the catch at the bottom of this function too, which you haven't 
>    changed here.

Ok, I'm no expert on exceptions or C++, and I've never seen a 
std::exception in the wild, but I found quite a few other places in 
BackupDaemon that looked like they could use the same treatment, so I 
fixed them all.

> 3. Could you not use IsTerminateWanted() rather than break to get of the 
>    loop?

It's not the same. IsTerminateWanted() is a signal that the whole of 
bbackupd needs to die ASAP, whereas if something goes badly wrong with the 
handler thread, we can just let it die and bbackupd keeps running. I think 
that's better.

I don't like using break, but the alternative seemed to be to set a flag 
and call continue, which looks worse to me. At least "break;" with a good 
comment has an intuitive meaning, whereas using "continue" to kill the 
thread is counter-intuitive, isn't it?

Please review for merge:

   svn diff -r 748:798 \
   http://bbdev.fluffy.co.uk/svn/box/chris/merge/bin/bbackupd/BackupDaemon.cpp

> 1. I'm guessing Ben might have something to say about the naming of the
> new struct.
>
> 2. This adds new options 'T' to list and 'A' to compare, but no code to
> handle them.

Well spotted, thanks! Fixed in r799. I'll wait for Ben to tell me what he 
wants the struct to be called before asking you to review again.

>> Please consider 766:767 for merge.
>
> 1. What if rDir to MakeFullPath is empty? If that's impossible it should
> be documented as a precondition.
>
> 2. Why are you changing copy construction of locals to assignment? Copy
> construction is better.

Fixed both. Please review for merge:

   svn diff -r 766:800 \
   http://bbdev.fluffy.co.uk/svn/box/chris/general/bin/bbackupd/BackupClientDirectoryRecord.cpp

By the way, why is copy construction better? More efficient?

> As far as I can see from my inbox this is the full list of outstanding 
> merges. If I've missed any please correct me. Again, sorry for the 
> delay.

Thanks again! OK if I send you some more patches for review this weekend?

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 |




More information about the Boxbackup-dev mailing list