[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