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

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

On 3/8/21 3:13 PM, Pierre Morel wrote:
> 
> 
> 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?

Alright, it's 2:1 for "success", so keep it if you want.

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