[PATCHES] Various CTDB bug fixes and improvements

Michael Adam obnox at samba.org
Tue Jan 12 18:17:34 UTC 2016


On 2016-01-12 at 16:55 +1100, Martin Schwenke wrote:
> I'm starting the new year by de-cluttering...  :-)
> 
> Attached are a number of patches that I've cherry-picked from across
> my various work-in-progress branches.  Some of of them are completely
> independent.  Others I consider to be obvious enough to push without
> the context of other patches that depend on them.

> [PATCH 01/10] ctdb-daemon: Drop the "schedule for deletion" messages to DEBUG level
> [PATCH 02/10] ctdb-recoverd: Fix some uninitialised memory issues
> [PATCH 03/10] ctdb-daemon: Don't leak memory if not using recovery lock
> [PATCH 04/10] ctdb-ipalloc: Fix a memory leak
> [PATCH 05/10] ctdb-scripts: Fix regression in updateip code
> [PATCH 06/10] ctdb-scripts: Clarify logic for success of interface monitoring
> [PATCH 07/10] ctdb-scripts: Superficial clean-ups to 10.interface
> [PATCH 08/10] ctdb-scripts: Don't remove temporary files before use
> [PATCH 09/10] ctdb-scripts: Use more unique temporary file names
> [PATCH 10/10] ctdb-tests: Fix some incorrect memory allocations
> 
> Please review and push.  Obviously feel free to push a subset if you
> object to some of them, since most are independent of each other.  :-)

Nice cleanup! I reviewed them and pushed to autobuild.

A few thoughts inline:

> From 7555f7e79a849e3b15ead4c141a4943193ce79b1 Mon Sep 17 00:00:00 2001
> From: Martin Schwenke <martin at meltin.net>
> Date: Mon, 11 Jan 2016 17:23:12 +1100
> Subject: [PATCH 02/10] ctdb-recoverd: Fix some uninitialised memory issues
> 
> The first element of these structures is a 32-bit PNN.  On 64-but
> systems ths field canbe followed by 32-bits of padding.  When the
> structures are copied this can cause uninitialised memory to be
> copied.
> 
> Signed-off-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/server/ctdb_monitor.c  | 1 +
>  ctdb/server/ctdb_recoverd.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/ctdb/server/ctdb_monitor.c b/ctdb/server/ctdb_monitor.c
> index d8eda2a..0a8273a 100644
> --- a/ctdb/server/ctdb_monitor.c
> +++ b/ctdb/server/ctdb_monitor.c
> @@ -134,6 +134,7 @@ static void ctdb_health_callback(struct ctdb_context *ctdb, int status, void *p)
>  	c.pnn = ctdb->pnn;
>  	c.old_flags = node->flags;
>  
> +	ZERO_STRUCT(rd);
>  	rd.pnn   = ctdb->pnn;
>  	rd.srvid = CTDB_SRVID_TAKEOVER_RUN_RESPONSE;
>  
> 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?...

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


> From 2e59c95c9a63ab38e22045ef9a0970fa2b10e1b8 Mon Sep 17 00:00:00 2001
> From: Martin Schwenke <martin at meltin.net>
> Date: Wed, 16 Dec 2015 19:18:49 +1100
> Subject: [PATCH 06/10] ctdb-scripts: Clarify logic for success of interface
>  monitoring
> 
> The current code uses so many shell idioms that it is difficult to
> follow.
> 
> Signed-off-by: Martin Schwenke <martin at meltin.net>
> ---
>  ctdb/config/events.d/10.interface | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/ctdb/config/events.d/10.interface b/ctdb/config/events.d/10.interface
> index 7445200..0cd1112 100755
> --- a/ctdb/config/events.d/10.interface
> +++ b/ctdb/config/events.d/10.interface
> @@ -152,13 +152,16 @@ monitor_interfaces()
>  
>  	done
>  
> -	$fail || return 0
> -
> -	$up_interfaces_found && \
> -	    [ "$CTDB_PARTIALLY_ONLINE_INTERFACES" = "yes" ] && \
> +	if $fail ; then
> +	    if $up_interfaces_found && \
> +		    [ "$CTDB_PARTIALLY_ONLINE_INTERFACES" = "yes" ] ; then
> +		return 0
> +	    else
> +		return 1
> +	    fi
> +	else
>  	    return 0
> -
> -	return 1
> +	fi
>  }
>  
>  # Sets: iface, ip, maskbits, family
> -- 
> 2.6.4

Two comments:

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

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

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160112/de483fcc/signature.sig>


More information about the samba-technical mailing list