Re: [kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Janosch Frank <frankja@linux.ibm.com>]

On 5/16/22 17:02, Steffen Eiden wrote:
> 
> 
> On 5/13/22 11:50, Janosch Frank wrote:
>> Let's also test for rc 0x3
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> I, however, have some nits below.
> 
>> ---
>>    s390x/uv-host.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++--
>>    1 file changed, 76 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 0f0b18a1..f846fc42 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -83,6 +83,24 @@ static void test_priv(void)
>>    	report_prefix_pop();
>>    }
>>    
>> +static void test_uv_uninitialized(void)
>> +{
>> +	struct uv_cb_header uvcb = {};
>> +	int i;
>> +
>> +	report_prefix_push("uninitialized");
>> +
>> +	/* i = 1 to skip over initialize */
>> +	for (i = 1; cmds[i].name; i++) {
>> +		expect_pgm_int();
>> +		uvcb.cmd = cmds[i].cmd;
>> +		uvcb.len = cmds[i].len;
>> +		uv_call_once(0, (uint64_t)&uvcb);
>> +		report(uvcb.rc == UVC_RC_INV_STATE, "%s", cmds[i].name);
>> +	}
>> +	report_prefix_pop();
>> +}
>> +
>>    static void test_config_destroy(void)
>>    {
>>    	int rc;
>> @@ -477,13 +495,68 @@ static void test_invalid(void)
>>    	report_prefix_pop();
>>    }
>>    
>> +static void test_clear_setup(void)
> maybe rename this to setup_test_clear(void)
> I initially mistook this function as a test and not a setup function for
> a test

Sure

> 
>> +{
>> +	unsigned long vsize;
>> +	int rc;
>> +
[...]
>>    static void setup_vmem(void)
>> @@ -514,6 +587,7 @@ int main(void)
>>    
>>    	test_priv();
>>    	test_invalid();
>> +	test_uv_uninitialized();
>>    	test_query();
>>    	test_init();
> IIRC this test must be done last, as a following test has an
> uninitialized UV. Maybe add a short comment for that here.

You're referring to the test_init()?

The test_clear() function must be done last but you're commenting under 
the test_init() call. So I'm a bit confused about what you want me to do 
here.

>>    
>