Re: [LTP] [PATCH RFC] ltp: define and use common clone helpers

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Mike Frysinger (vapier@gentoo.org):
> On Monday 21 September 2009 19:06:44 Serge E. Hallyn wrote:
> > Define ltp_clone() and ltp_clone_malloc() in libltp, and convert existing
> > clone usages to them.  (clone04 can't use it bc it wants to pass NULL,
> > which ltp_clone() will for many arches convert to NULL+stacksize-1).
> 
> so have the code handle NULL specially:
> (stack ? stack + stack_size - 1 : NULL)

<grumble> yeah that occurred to me but I was rebelling against the clone04.c
code...  But I guess I should.

> > +ltp_clone(unsigned long clone_flags, void *stack, int stack_size,
> > +		int (*fn)(void *arg), void *arg)
> > +{
> > +	int ret;
> > +
> > +#if defined(__hppa__)
> > +	ret = clone(fn, stack, clone_flags, arg);
> > +#elif defined(__ia64__)
> > +	ret = clone2(fn, stack, stack_size, clone_flags, arg, NULL, NULL, NULL);
> > +#else
> > +	ret = clone(fn, stack + stack_size - 1, clone_flags, arg);
> > +#endif
> > +
> > +	if (ret == -1)
> > +		perror("clone");
> 
> we cant be sure why the higher layers are calling clone.  maybe the args given 
> expect the clone() call to fail.  so we dont want any perror() invocation 
> here.

Makes sense.

> > +/***********************************************************************
> > + * ltp_clone_malloc: also does the memory allocation for clone.
> > + * Experience thus far suggests that one page is often insufficient,
> > + * while 4*getpagesize() seems adequate.
> > + ***********************************************************************/
> 
> a malloc() function implies you should be giving it a size.  i think there 
> should be another helper here.
> ltp_clone_malloc() - takes a size
> ltp_clone_quick() - calls ltp_clone_malloc() with getpagesize() * 4
> 
> or a better name than "quick" ...
> 
> > +int
> > +ltp_clone_malloc(unsigned long clone_flags, int (*fn)(void *arg), void
> >  *arg)
> 
> i think argument order should be consistent.  i.e. have all ltp_clone_* calls 
> start with (flags, func, arg) and then the malloc/etc... calls can add on 
> (..., size) and (..., size, buffer).

makes sense.

> > +	void *stack = malloc (stack_size);
> 
> no spacing around function calls
> 
> > +	if (!stack) {
> > +		perror("malloc");
> > +		return -1;
> > +	}
> 
> since people are linking in -lltp to get these clone helpers, we can assume 
> the tst_* funcs exist.  so this should invoke one of them with TBROK|TERRNO.

True.

> > +	ret = ltp_clone(clone_flags, stack, stack_size, fn, arg);
> > +
> > +	if (ret == -1) {
> > +		perror("clone");
> > +		free(stack);
> > +	}
> 
> same issue as the other func -- dont call perror()
> 
> i think we should make sure to save/restore errno across the free() invocation 
> so that the caller gets the result from clone() ...

Good point.

> otherwise this looks great.  thanks for doing the footwork here.
> -mike

Will hopefully whip up a new patch later this week and resend.

thanks,
-serge

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list