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

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1.1: Type: Text/Plain, Size: 2579 bytes --]

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)

> +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.

> +/***********************************************************************
> + * 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).

> +	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.

> +	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() ...

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 401 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry® 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-12, 2009. Register now!
http://p.sf.net/sfu/devconf

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list