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

[Swen Schillig]

On Tue, 2018-03-20 at 12:26 +1100, Martin Schwenke wrote:
> 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
> 

Updated patch-set.

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb_daemon_read_cb.patch
Type: text/x-patch
Size: 2596 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180320/8a4fa988/ctdb_daemon_read_cb.bin>