Refactor dbwrap_ctdb a bit

[Volker Lendecke]

On Fri, Nov 23, 2012 at 01:36:10PM +0100, Michael Adam wrote:
> On 2012-11-22 at 14:50 +0100, Volker Lendecke wrote:
> > Hi!
> > 
> > Attached find a patch that refactors dbwrap_ctdb a bit. A
> > while ago, the parse_record dbwrap backend operation was
> > added. dbwrap_fetch() at the dbwrap toplevel was converted
> > to use parse_record, so that the fetch backend operation
> > became unnecessary. All but the dbwrap_ctdb backends were
> > converted. dbwrap_ctdb however does a fake parse based
> > internally on the fetch operation. This leads to allocations
> > and memcpy operations where they are not strictly necessary.
> > 
> > The attached patchset converts dbwrap_ctdb to do a proper
> > parse operation avoiding the memory allocations in the local
> > cases.
> > 
> > Comments?
> 
> Some first comments on details:

Thanks for looking!

> - [PATCH 03/16] s3: Remove header==NULL code from db_ctdb_marshall_record
> 
>   I don't like that the fact that there is currently no caller
>   that passes header == NULL into the function is used as a
>   justificatino for removing all checks for header == NULL.
>   We could at least add an early if (header == NULL) { return...  }
>   guard.
> 
>   Currently, this is OK, but in the past, there were callers
>   that handedin header == NULL.

And if those callers re-appear we could certainly add this
handling again. The reason for this patch is to remove
unnecessary complexity from the code. This (header==NULL) if
statement made me wonder what the use case for this would
be. My assumption was that there was always a header to
marshall, and the header==NULL thing made me wonder what
other things might have to be marshalled. I found that only
full, "correct" ctdb records including the header were
marshalled right now, so I removed that piece. If we want to
plan for future extension current code, we can certainly do
that, I won't block it. But I prefer simpler code that makes
you scratch your head no more than strictly necessary.

> - [PATCH 08/16] s3: Slightly simplify db_ctdb_marshall_loop_next
> 
>   Same comment as above. It makes it less obvious how to use the functions.
>   This is not wrong currently, but gives me a bad feeling.

I'm not sure here either. We have full records including
headers in the marshall buffer, and I would argue that this
could wenn show up in the code.

> - [PATCH 05/16] s3: Add db_ctdb_ltdb_parse
> 
>   Wouldn't it be more natural to hand the db_ctdb_context into
>   the function instead of a tdb_context?
> 
>   All callers hand in db->wtdb->tdb anyways.

Also a minor thing. I wanted to pass down the mininum
necessary information. All this function looks at is a ltdb,
which is a tdb_context. Sure, if in the future this will
ever have to look at more context, we will pass down a
higher-level structure. But at this moment just the tdb.
This is a purely internal function, not an API that is
published anywhere else yet, so for me local simplicity
counts more than generality.

None of the points are really sticking points for me, but I
did them on purpose :-)

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de