Re: [kvm-unit-tests PATCH v3 1/5] s390x: css: Store CSS Characteristics

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

On 2/26/21 10:50 AM, Janosch Frank wrote:
> On 2/18/21 6:26 PM, Pierre Morel wrote:

>>   
>> +/*
>> + * CHSC definitions
>> + */
>> +struct chsc_header {
>> +	u16 len;
>> +	u16 code;
> 
> uint*_t types please

OK

> 
>> +};

>> +static int check_response(void *p)
>> +{
>> +	struct chsc_header *h = p;
>> +
>> +	if (h->code == CHSC_RSP_OK) {
>> +		report(1, "CHSC command completed.");
> 
> I'm not a big fan of using integer constants for boolean type arguments.

hum, right, I will rework all these reports that should not appear here
anyway

> 
>> +		return 0;
>> +	}
>> +	if (h->code > CHSC_RSP_MAX)
>> +		h->code = 0;
>> +	report(0, "Response code %04x: %s", h->code, chsc_rsp_description[h->code]);
>> +	return -1;
>> +}
>> +
>> +int chsc(void *p, uint16_t code, uint16_t len)
>> +{
>> +	struct chsc_header *h = p;
>> +	int cc;
>> +
>> +	report_prefix_push("Channel Subsystem Call");
>> +	h->code = code;
>> +	h->len = len;
>> +	cc = _chsc(p);
>> +	switch (cc) {
>> +	case 3:
>> +		report(0, "Subchannel invalid or not enabled.");
>> +		break;
>> +	case 2:
>> +		report(0, "CHSC subchannel busy.");
>> +		break;
>> +	case 1:
>> +		report(0, "Subchannel invalid or not enabled.");
>> +		break;
> 
> I don't think that this is how we want to handle error reporting in lib
> files.
> 
> Please don't use report for library error reporting if it's not needed.
> 
> Most of the times you should return an error code or simply
> abort()/assert() if for instance a library init function fails and you
> can assume that most of the test code is dependent on that librarie's
> initialization. Sometimes report_abort() is also ok.
> 
> Test code should not be part of the library if possible!

Yes, will take care of that.

>>   static struct {
>>   	const char *name;
>>   	void (*func)(void);
>>   } tests[] = {
>> +	/* The css_init test is needed to initialize the CSS Characteristics */
>> +	{ "initialize CSS (chsc)", css_init },
> 
> Is css_init() really a test or does it only setup state for further tests?

Yes it is a test too
I rework that.

> 
>>   	{ "enumerate (stsch)", test_enumerate },
>>   	{ "enable (msch)", test_enable },
>>   	{ "sense (ssch/tsch)", test_sense },
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen