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

[Martin Doucha]

On 14. 04. 22 12:25, Cyril Hrubis wrote:
> 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"

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

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

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic