[PATCHES] Various CTDB bug fixes and improvements

[Martin Schwenke]

On Tue, 12 Jan 2016 19:17:34 +0100, Michael Adam <obnox at samba.org>
wrote:

> On 2016-01-12 at 16:55 +1100, Martin Schwenke wrote:

> ...
> > diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
> > index 1d63526..c89649a 100644
> > --- a/ctdb/server/ctdb_recoverd.c
> > +++ b/ctdb/server/ctdb_recoverd.c
> > @@ -1650,6 +1650,7 @@ static bool do_takeover_run(struct ctdb_recoverd *rec,
> >  	 * wait for replies since a failure here might cause some
> >  	 * noise in the logs but will not actually cause a problem.
> >  	 */
> > +	ZERO_STRUCT(dtr);
> >  	dtr.srvid = 0; /* No reply */
> >  	dtr.pnn = -1;  
> 
> I thought a bit about this.
> 
> Usually, I would try to use
> 
> dtr = {
> 	.srvid = 0;
> 	.pnn = -1;
> };
> 
> It is cheaper, but this does not initialize the padding.
> Then again, the question is why the padding initialization
> is so important. If we send unmarshalled data with padding
> across the wire, then the receiving end better have the
> same padding, and hence the padding should not matter
> (well, I guess this is naive...).

> My thought is just: If we need ZERO_STRUCT to prevent
> sending uninitialized data, shouldn't we take that as
> an indication that we should rather do marshalling?...

Right.  Except that uninitialised data is memcpy()ed "onto the wire",
so there's a potential data leak, if anyone is watching... and valgrind
complains.  :-)

For the moment:

* This is old code, which uses the old structs on wire approach.

* Although Amitay has written marshalling code for all of the protocol
  elements, he has also gone to great lengths to keep the protocol
  unchanged for now.  So his current marshalling code will also
  maintain the padding.

* I might be able to switch these places in the code to use the new
  marshalling.  However, that's a "slippery slope", since I could
  probably spend the next month doing that.  I was actually trying to
  use valgrind to track down a memory corruption issue in my WIP, but
  between the noise from these issues and Debian's valgrind having a
  low signal to noise ratio right now (keeps complaining about errors
  reading debuginfo in libc6, reported), I decided on a quick fix to get
  stuff done.  ;-)

> So I took the patch as is, but ZERO_STRUCT is rather
> conceptually a little wrong for initialization. :-)

Thanks!

The new marshalling code will be used more and more, as code gets
cleaned up and separated out.  When everything uses the new marshalling
code then we can do "the tablecloth trick": throw away the
hand-generated marshalling code that effectively still puts the structs
on the wire and replace it with a new "protocol" that has
auto-generated marshalling code.  Then we can properly version the
thing, have heterogeneous clusters, ...

> Two comments:
> 
> - attached find a follow-up patch that further
>   (imho) simplifies it by reducing indentation.

I'm still thinking about which version is clearer...  :-)

> - It would be *SOOO* great if we could stop using
>   that 8-char tab + 4 chars as half tab indentation!!! :-)

Separate reply...

peace & happiness,
martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 173 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160113/c9b598f1/attachment.sig>