CTDB: Small patch set fixing memory leak and other things for ctdb_daemon_read_cb

[Swen Schillig]

On Fri, 2018-03-16 at 20:46 +1100, Martin Schwenke wrote:
> On Tue, 13 Mar 2018 10:06:56 +0100, Swen Schillig via samba-technical
> <samba-technical at lists.samba.org> wrote:
> 
> > The attached file contains 3 small patches fixing a memory leak,
> > removes not require checks and is cleaning up the code for column
> > length, function call formatting and using the right logging
> > macros.
> 
> For the 1st commit, the commit message says:
> 
>   Within ctdb_daemon_read_cb the provided data is checked for sanity,
>   e.g. correct size and content. This is not required because it was
>   done already by the caller.
> 
> You might as well name the caller.
> 
> I assume you mean queue_process() in ctdb/common/ctdb_io.c.  However,
> I
> don't see where that function confirms that it has read a full
> header.
That is the pkt_size,
we do 
	pkt_size = *(uint32_t *)queue->buffer.data;
and
 	if (queue->buffer.length < pkt_size) {
		...
		return;
	}
in queue_process before we're calling the callback.
which is the same as 
 	hdr = (struct ctdb_req_header *)data;
	if (cnt != hdr->length) 
having struct ctdb_req_header as
	struct ctdb_req_header {
		uint32_t length;
		...
	}

> 
> In the 2nd commit, since the debug message is very generic and is
> meant
> for debugging, please use DBG_DEBUG(), since this logs the function
> name, instead of D_DEBUG().
I only learned recently, that I should replace those "old-fashioned"
macros by the newer-/shorter-ones as they do include the name already.
...and if you're looking at 

#define D_DEBUG(...)		DEBUG(DBGLVL_DEBUG,	(__VA_ARGS__))
and 
#define DEBUG( level, body ) \
  (void)( ((level) <= MAX_DEBUG_LEVEL) && \
	  unlikely(DEBUGLEVEL_CLASS[ DBGC_CLASS ] >= (level))		\
       && (dbghdrclass( level, DBGC_CLASS, __location__, __FUNCTION__ )) \
       && (dbgtext body) )

this seems to be the case, right ?
> 
> We introduced the D_*() macros for CTDB because we do a lot of
> logging
> of cluster/node state changes.  These messages are usually unique and
> are often meant to be read by an admin or support person, so don't
> need
> the clutter of the function name.  For the deeper debugging messages
> we
> need a little more context.
> 
> I think the 3rd commit looks good.
> 
> peace & happiness,
> martin

Please have a look again.

Thanks in advance.

Cheers Swen.