[LTP] [PATCH v2 2/3] KVM test infrastructure

[Cyril Hrubis]

Hi!
> >> +/*
> >> + * These constants must match the value of corresponding constants defined
> >> + * in tst_res_flags.h.
> >> + */
> >> +#define TPASS	0	/* Test passed flag */
> >> +#define TFAIL	1	/* Test failed flag */
> >> +#define TBROK	2	/* Test broken flag */
> >> +#define TWARN	4	/* Test warning flag */
> >> +#define TINFO	16	/* Test information flag */
> >> +#define TCONF	32	/* Test not appropriate for configuration */
> > 
> > The tst_res_flags.h header has been created so that there will be no
> > need to copy these definitions around like this. Why not just include
> > the header?
> 
> I didn't #include the tst_res_flags.h because the main LTP include
> directory is not passed to the preprocessor when compiling KVM payload.
> I'd have to do it like this:
> #include "../../../../include/tst_res_flags.h"

And I guess that this is on purpose right?

As we can easily add it by adding -I$(top_src_dir)/include/ to the
CFLAGS.

> >> +void tst_res_(const char *file, const int lineno, int result,
> >> +	const char *message);
> >> +#define tst_res(result, msg) tst_res_(__FILE__, __LINE__, (result), (msg))
> >> +
> >> +void tst_brk_(const char *file, const int lineno, int result,
> >> +	const char *message) __attribute__((noreturn));
> >> +#define tst_brk(result, msg) tst_brk_(__FILE__, __LINE__, (result), (msg))
> > 
> > I think that it may be slightly better to name these kvm_res() and
> > kvm_brk() to make clear that these are used in the guest context and
> > that the implementation is actually different. I guess that it would
> > make sense to have all the functions called from the guest prefixed with
> > kvm_ just to make the boundary clearer.
> 
> My idea was different:
> - kvm_* prefix for low-level arch-specific functions
> - tst_* prefix for reimplementation of standard LTP library functions
> and any other platform-independent helper functions
> 
> tst_res() and tst_brk() currently lack the printf-like formatting
> features (and the TERRNO/TTERRNO/TRERRNO flags are not applicable) but
> otherwise they do the exact same thing as the standard LTP functions.
> I'm planning to add some limited printf-like formatting later since
> sprintf() implementation will be needed eventually anyway.

Ack.

> >> +void *tst_heap_alloc_aligned(size_t size, size_t align)
> >> +{
> >> +	uintptr_t addr = (uintptr_t)heap_end;
> >> +	void *ret;
> >> +
> >> +	addr += align - 1;
> >> +	addr -= addr % align;
> >> +	ret = (void *)addr;
> >> +	addr += size + 3;
> >> +	addr -= addr % 4;
> >> +	heap_end = (char *)addr;
> >> +	return ret;
> >> +}
> >> +
> >> +void *tst_heap_alloc(size_t size)
> >> +{
> >> +	void *ret = heap_end;
> >> +
> >> +	size += 3;
> >> +	size -= size % 4;
> >> +	heap_end += size;
> >> +	return ret;
> > 
> > We do have a few similar pieces scattered around the code, can put the
> > code into a macro that would make the code slightly easier to
> > understand?
> > 
> > We do have LTP_ALIGN() in tst_common.h but I guess that we do not want
> > to include it here in the guest library.
> 
> I guess that copying LTP_ALIGN() to the KVM headers would be useful.

Can you please respin the patchset with this change then?

-- 
Cyril Hrubis
chrubis@suse.cz