[PATCH] ctdb: Replace mem-alloc/assign by move/memdup

[Andrew Bartlett]

On Fri, 2018-03-16 at 09:14 +0100, Swen Schillig via samba-technical
wrote:
> On Fri, 2018-03-16 at 20:21 +1300, Andrew Bartlett via samba-technical
> wrote:
> > On Wed, 2018-03-14 at 16:10 +1100, Martin Schwenke via samba-
> > technical
> > wrote:
> > > On Fri, 09 Mar 2018 09:05:25 +0100, Swen Schillig via samba-
> > > technical
> > > <samba-technical at lists.samba.org> wrote:
> > > 
> > > > Please review and push if OK.
> > > 
> > > Reviewed-by: Martin Schwenke <martin at meltin.net>
> > 
> > CTDB isn't my area, but I'm not sure if I like this.  The assignment
> > pattern is type safe (and really ends up dong the same thing),
> > however
> > the memdup pattern is not type safe, because it takes and returns a
> > void *.  
> > 
> > In particular because sizeof(struct ctdb_call) is used, not
> > sizeof(*call), the type could be changed and then the memdup would be
> > wrong, and nobody would notice at compile time.
> > 
> > It also means the memory won't have the right type in the talloc
> > name,
> > but the line number instead, which means you can't do
> > talloc_get_type()
> > later. 
> > 
> > So I'm going to say 'no' sorry, because if it were 'my' code, I
> > wouldn't want this and while I know Martin has said yes, I think this
> > is a poor pattern. 
> > 
> > (A way out would be to extend talloc to provide a structure dup
> > function that uses typeof() or such tricks and so can't get it
> > wrong). 
> > 
> > Sorry,
> > 
> > Andrew Bartlett
> 
> Hmm, well nothing which couldn't be fixed by 
> 	sizeof(*call)
> and
> 	talloc_set_name()
> 
> but I guess that would take all the good out of it and turn it into a
> "I want to change something for the change sake" patch.

Not quite (and it is worth learning about these special features of
talloc, they are there to protect us). 

To be clear, even that wouldn't make talloc_memdup() return the correct
type.  One of the things that talloc() does is return the type given in
the parameter, so you can't assign it to the wrong type, avoiding a
common hazard where the arguments to (eg) malloc() don't match the
variable being assigned to. 

Andrew Bartlett
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba