[Box Backup-dev] COMMIT r348 - in box/chris/bb-save-state: . bin/bbackupd lib/common lib/server

[Ben Summers]

On 30 Jan 2006, at 13:05, Martin Ebourne wrote:

> Ben Summers <ben@fluffy.co.uk> wrote:
>> I'm probably being too picky here. I wonder what Martin thinks.
>
> You asked for my opinion so - uh oh! - you get it!

You are very obliging.

>
> No, I don't think you're being too picky, it's just unfortunate  
> this has landed on Chris as the only person managing to spend time  
> on it at the moment.

Yes, time is always an issue. I am very busy right now, so am trying  
to just review stuff and then spend a few hours packaging and testing  
a release candidate when all the changes are in.

>
>>> svn diff -r 336:349 http://bbdev.fluffy.co.uk/svn/box/chris/bb- 
>>> save- state
>
> I reviewed 336:350 to catch the new changes.
>
>> I would have removed the operator overloading from Archive, and   
>> personally would have just required that classes derived from  
>> Archive  only provide Read(void*,int length) and Write(void*,int  
>> length)  functions, rather than doing absolutely everything, and  
>> made base  class Get and Put inline functions which call those.
>
> I like your second plan rather than this one. Seems Chris got the  
> wrong end of the stick on this one. Fortunately it's all good  
> because what he did is closer to your second plan anyway.
>
>> 	-- #include "Archive.h" -> class Archive;  ?
>
> Agreed.
>
>> As I understand it, I just need to adjust the infrastructure  
>> building  to run ./bootstrap, and then include all the new files  
>> from SVN in  the root, along with the generated configure?
>
> Not sure what you mean by root. There's new files in subdirs too. I  
> think you'll do the right thing though.

The top level of the archive. So the stuff at the same level as bin,  
lib and test.

>
>> Onto the code... I wonder if it would be better to just delete  
>> the  Archive class, and rename IOStreamArchive to Archive. I don't  
>> quite  see what this gives us, except added complexity and the  
>> overhead of  virtual function calls, given that you can get all  
>> the flexibility  you need by simply choosing your IOStream  
>> appropriately. Then Archive  becomes a very simple class, calls to  
>> which compile down to calls to  the underlying IOStream object.
>
> I agree completely. The abstraction for Archive is at the wrong  
> level anyway. Thinking about possible alternatives you might want  
> to slot in here and an obvious one that springs to mind is a  
> database store(*). Using the current Archive interface you couldn't  
> do that. Really the Archive interface is an IO interface and as  
> such should just be replaced with an IO stream.
>
> It's further confused by the Box IOStream implementation. Although  
> it's called an IOStream, when compared to standard C++ streams it's  
> really a streambuf. Protocol is more like the C++ iostream.

:-) I called them IOStreams as they're streams of data, which go in  
an out. They don't intepret them, they just deal with the bytes. Then  
other stuff does things with the bytes.

>
> Long term I think there'd be mileage in refactoring Archive and  
> Protocol to remove duplication, but that's way too much work at the  
> moment.

Agreed.


[snip]

>
>> OTOH, perhaps we should just merge it and get on with things?
>
> I think it would be great if Chris can make the fixes you suggest  
> since at this stage they shouldn't be too tricky. On the other hand  
> if he's had enough already then they aren't his changes and we  
> should be thankful for the work he's already put in. :)
>
> In terms of changes still not in trunk, I don't think the type  
> changes have been merged yet. I think Chris did fix some/all of  
> them following review comments though. Have we a new diff to review  
> for that one before merge?

I think that's in, in the end it was only about 10 lines changed. Or  
perhaps I'm getting confused. We do need a way to track this, I'm  
sure the wiki could help. Or I could install Trac on the SVN server.

>
> And are some windows changes still pending?

I don't think so, unless anyone wants to fix Charles' list of Win32  
issues before the release candidate.

>
> (*) Sometime in the future it might make sense to replace this  
> custom state file and the whole of Berkeley DB hell with sqlite,  
> which I think would be much more manageable and give us added  
> flexibility for new features (maybe inotify).

Yes, maybe my database interface will be of some use after all,  
although you do know my views on dependencies. Once we've got 0.10,  
we can list all the stuff we want to do in the next version.

Ben