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

[Pierre Morel <pmorel@xxxxxxxxxxxxx>]

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