Refactor dbwrap_ctdb a bit

[Michael Adam]

On 2012-11-23 at 13:58 +0100, Volker Lendecke wrote:
> On Fri, Nov 23, 2012 at 01:36:10PM +0100, Michael Adam wrote:
> > On 2012-11-22 at 14:50 +0100, Volker Lendecke wrote:
> > > 
> > > 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.

Well, what surprised me is mostly that the patches remove a
safety guard (against NULL dereference). Chances are that if
we should really use them with NULL input in the future, we
might learn the hard way that we need to add checks, because
we assumed that the functions are safe to use.

I won't insist on keeping the checks, but would feel more
comfortable if we could add fat warning to function header
comments. I can add those.

> > - [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.

Hmmm.

For me, what counts is the simplicity for the caller instead of for
the function implementation, and the naturality of the names and
concepts. The "ltdb" is not just a tdb context, but it is the
local tdb that belongs to a ctdb db context. So even if this
is just an internal function, if I read the name
db_ctdb_ltdb_do_somehting(), my reflex is to hand a db_ctdb_ctx
in and to expect the function to do something on the db's ltdb.
If I want a function that operates on any tdb, I'd not call it
foo_ltdb_bar but foo_tdb_bar. But given the fact, that all
callers currently hand in db_ctx->wtdb->tdb, I'd still say that the
ltdb naming would be more natural and less surprising for the
user of the function. If we need to broaden up the scope of
callers in the future, (e.g. handing in general tdbs), then we
could still factor the general purpose tdb-level function later
or so. (Hey, here I am using your argument of the NULL check
above... ;-)

I apologize for spreading out this admittedly minor point, but
for me it is always very important to have the functions as
intuitive and natural as possible, so I wanted to describe what I
found the most convenient variant, and why.

> None of the points are really sticking points for me,

same here

> but I did them on purpose :-)

I took that for granted... :)

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121123/11ea56d0/attachment.pgp>