Re: [kvm-unit-tests PATCH v4 2/6] s390x: css: simplifications of the tests

[Pierre Morel <pmorel@linux.ibm.com>]

On 3/1/21 4:00 PM, Janosch Frank wrote:
> On 3/1/21 12:47 PM, Pierre Morel wrote:
>> In order to ease the writing of tests based on:

...snip...

>> -static void test_sense(void)
>> +static bool do_test_sense(void)
>>   {
>>   	struct ccw1 *ccw;
>> +	bool success = false;
> 
> That is a very counter-intuitive name, something like "retval" might be
> better.
> You're free to use the normal int returns but unfortunately you can't
> use the E* error constants like ENOMEM.

hum, I had retval and changed it to success on a proposition of Thomas...
I find it more intuitive as a bool since this function succeed or fail, 
no half way and is used for the reporting.

other opinion?


> 
>>   	int ret;
>>   	int len;
>>   
>>   	if (!test_device_sid) {
>>   		report_skip("No device");
>> -		return;
>> +		return success;
>>   	}
>>   
>> -	ret = css_enable(test_device_sid, IO_SCH_ISC);
>> -	if (ret) {
>> -		report(0, "Could not enable the subchannel: %08x",
>> -		       test_device_sid);
>> -		return;
>> +	if (!css_enabled(test_device_sid)) {
>> +		report(0, "enabling subchannel %08x", test_device_sid);
>> +		return success;
>>   	}
>>   
>> -	ret = register_io_int_func(css_irq_io);
>> -	if (ret) {
>> -		report(0, "Could not register IRQ handler");
>> -		return;
>> -	}
>> -
>> -	lowcore_ptr->io_int_param = 0;
>> -
>>   	senseid = alloc_io_mem(sizeof(*senseid), 0);
>>   	if (!senseid) {
>>   		report(0, "Allocation of senseid");
>> -		goto error_senseid;
>> +		return success;
>>   	}
>>   
>>   	ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI);
>> @@ -129,21 +120,34 @@ static void test_sense(void)
>>   	report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x",
>>   		    senseid->reserved, senseid->cu_type, senseid->cu_model,
>>   		    senseid->dev_type, senseid->dev_model);
>> +	report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type,
>> +		    senseid->cu_type);
>>   
>> -	report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x",
>> -	       (uint16_t)cu_type, senseid->cu_type);
>> +	success = senseid->cu_type == cu_type;
>>   
>>   error:
>>   	free_io_mem(ccw, sizeof(*ccw));
>>   error_ccw:
>>   	free_io_mem(senseid, sizeof(*senseid));
>> -error_senseid:
>> -	unregister_io_int_func(css_irq_io);
>> +	return success;
>> +}
>> +
>> +static void test_sense(void)
>> +{
>> +	report(do_test_sense(), "Got CU type expected");
>>   }
>>   
>>   static void css_init(void)
>>   {
>>   	report(!!get_chsc_scsc(), "Store Channel Characteristics");
>> +
>> +	if (register_io_int_func(css_irq_io)) {
>> +		report(0, "Could not register IRQ handler");
>> +		return;
> 
> assert() please

Yes.

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen