[Box Backup] Code in SVN -- review notes

[Martin Ebourne]

Hi there,

Been on holiday for a week, so just got to this.

On Mon, 2005-10-31 at 18:25 +0000, Ben Summers wrote:
> Thanks for all the efforts of the contributors. I notice none of you  
> have bothered adding a decent CONTRIBUTORS file, this must be rectified.

That's the sort of thing that can be tidied up once the main stuff is
all out of the way. There's likely to be other doc stuff too.

> martin/solaris
> ==============
> lib/server/Protocol.cpp -- can we avoid those extra memcpys? Is it  
> actually causing a problem with some processor without the ability to  
> do non-word aligned loads? Can we make it conditional on being run on  
> one of those processors using PLATFORM_ALIGN_INT?

The memcpys are needed for processors which cannot load/store 32 bit
words on a random byte boundary. This includes sparc, arm, and others.

They already are conditional on PLATFORM_ALIGN_INT so I'm not sure what
you're after there.

> martin/xattr
> ============
> xattr is also available on Darwin, so the name of the test is  
> probably misleading.

Ok. I wasn't sure where else they are available, or even if they would
be compatible. If we test on darwin/etc and they work then obviously we
can change the name. Probably the test was renamed as part of the
autoconf changes anyway.

> My worries is that there may be quite a bit of xattr data, and the  
> way BackupClientFileAttributes are used assumes that they are  
> relatively small. I think they would be better represented as a  
> stream. But then, we need to write the code to support multiple streams.

Ah, ok, I wasn't aware of that. Previously you said that adding xattr
would just need a couple of new functions, and that looked the likely
place to me. Agreed the xattrs can be potentially large (though I've not
seen any large yet).

> Under Darwin, it'll pick up the resource fork as an xattr. This could  
> be quite large!

Ah, ok.

> A minor point, but in the format of the structure, you would have to  
> store a zero for the xattr size if it was extended afterwards. Also  
> if it was stored here, perhaps I would prefer the ability to stack  
> multiple attribute records, so you'd get a standard UNIX one and  
> another xattr one?

Yes, the format I'm using there is not my preferred format. It was done
so as to be both forward and backward compatible with both client and
server. Anything else will break that compatibility, and I didn't want
to do that yet.

My preferred format is as you suggest, having different kinds of
attribute record and the ability to stack them arbitrarily.

> I would be happy for this to be committed, as long as there was a  
> commitment (!) to revisit this code when multiple streams are  
> available in backup files.

No problem. I was intending to come back to this and redo the attribute
record format as above anyway. Streams sounds good too as long as it
will be similarly efficient.

> martin/autoconf
> ===============
> You appear to have removed my code which means someone has to take  
> positive action to working with old versions of OpenSSL. I'd prefer  
> that stayed -- it's a nasty hack to do the stuff I need with older  
> OpenSSLs.

Unless I'm missing something, I think it is covered by this section of
configure.ac:

## Check for Open SSL, use old versions only if explicitly requested
AX_CHECK_SSL(, [AC_MSG_ERROR([[OpenSSL is not installed but is
required]])])
AC_ARG_ENABLE(
  [old-ssl],
  [AC_HELP_STRING([--enable-old-ssl],
                  [Allow use of pre-0.9.7 Open SSL - NOT RECOMMENDED,
read the documentation])])
AC_CHECK_LIB(
  [crypto],
  [EVP_CipherInit_ex],, [
  if test "x$enable_old_ssl" = "xyes"; then
    AC_DEFINE([HAVE_OLD_SSL], 1, [Define to 1 if SSL is pre-0.9.7])
  else
    AC_MSG_ERROR([[found an old (pre 0.9.7) version of SSL.
Upgrade or read the documentation for alternatives]])
  fi
  ])

Yes, I agree, M4 is not the nicest of languages to work with. But then
nor is perl. :) A bit of practice and I found it not too bad.

Anyhow, the upshot of the above is that you have to explicitly give
--enable-old-ssl to use pre-0.9.7.

> infrastructure/BoxPlatform.pm - license slipped in!

Ah, good spot! Fixed, will check in soon.

> I think it looks OK (reading the diffs carefully) but I'm not sure  
> how to actually compile it. We need to try it out on "all" platforms.

Yes, of course.

> Also, how do we go about making a distribution? It'd be nice to have  
> something in docs/.

Ok. Most projects don't document the maintainer stuff in the distro
because users (installers) don't need it. Obviously the web docs will
need updating to use the autoconf style configure options, I can do
that. As for maintainer stuff it's really easy:

1. Check out in svn.
2. Run ./bootstrap
3. Do ./configure && make install in the normal way

I notice that bootstrap has lost +x from the patch, I'll fix that in
svn.

I would like as many people as possible to try this out and give me
feedback. You will need autoconf and automake installed to run
bootstrap, but neither to run configure or make. I'm using these
versions, though hopefully it will work with others:

autoconf-2.59-5
automake-1.9.5-1

> nick/win
> ========
> To be honest, this scares me. Also, the DOS line endings screw up the  
> diffs. Something to look at in more detail another day, I'm afraid.  
> If someone would like to commit one with the line endings The  
> Invisible Sky Giant intended us to use, that would be lovely.

As I mentioned in a previous mail I did check in a dos-unix cleaned up
copy under martin/trunk, so you can use that just for now.

svn diff http://bbdev.fluffy.co.uk/svn/box/ben/box0_08 http://bbdev.fluffy.co.uk/svn/box/martin/test

should do the trick.

My fixes above checked in at r35.

Cheers,

Martin.