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

Martin Schwenke martin at meltin.net
Tue Mar 20 01:26:11 UTC 2018


On Mon, 19 Mar 2018 10:33:25 +0100, Swen Schillig <swen at vnet.ibm.com>
wrote:

> On Mon, 2018-03-19 at 14:35 +1100, Martin Schwenke via samba-technical
> wrote:
> > On Fri, 16 Mar 2018 11:50:27 +0100, Swen Schillig <swen at vnet.ibm.com>
> > wrote:

> > > 
> > > 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;
> > > 		...
> > > 	}  
> > 
> > It isn't the same.  :-(  
> You're right.
> I need to add that one check back.

Excellent.

As I said in my original comment, I think you should name the caller
(i.e. queue_process()) in the commit message.  That gives someone less
familiar with the code a starting point for a review.  They can go
straight to queue_process() and see how the context in which the
callback is called. If they want to they can also look more broadly
with the file that contains it and confirm that the associated setup
function is indeed used to setup the queue.

I also vaguely feel like the space/wrapping cleanups don't need to be
in the same commits as the other stuff.  However, I'm not going to
bike-shed on that. Perhaps others will feel more strongly.

With a commit message that names the calling function:

Reviewed-by: Martin Schwenke <martin at meltin.net>

> > > 
> > > 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,	(__V
> > > A_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 ?  
> > 
> > Please read dbghdrclass() and ctdb/common/logging.c.

> Removed the cleanup.

OK, but I think that understanding what I meant would be valuable.  ;-)

peace & happiness,
martin



More information about the samba-technical mailing list