Re: [Qemu-devel] [PATCH 3/9] s390x/css: be more consistent if broken beyond repair

[Thomas Huth]

On 31.08.2017 08:10, Thomas Huth wrote:
> On 30.08.2017 18:36, Halil Pasic wrote:
>> If we detect that the internally manged state of the subchannel
>> is broken beyond repair while in do_subchannel_work in case of
>> virtual we just abort the operation and pretend all went well,
>> while in case of pass-through we honor the situation with -ENODEV
>> which results in cc 3 for the instruction whose handler triggered
>> the call.
>>
>> Let's be consistent on this and do the -ENODEV also for virtual
>> subchannels.
>>
>> Signed-off-by: Halil Pasic <address@hidden>
>> Acked-by: Pierre Morel<address@hidden>
>> ---
>>  hw/s390x/css.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 0822538cde..bc47bf5b20 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1078,7 +1078,7 @@ int do_subchannel_work_virtual(SubchDev *sch)
>>          sch_handle_start_func_virtual(sch);
>>      } else {
>>          /* Cannot happen. */
>> -        return 0;
>> +        return -ENODEV;
>>      }
>>      css_inject_io_interrupt(sch);
>>      return 0;
> 
> First, I think ENODEV is not really a good choice here since there
> certainly was a device. So maybe use EINVAL or EBADR or something else
> instead?
>
> Second, while return an error code is certainly better than returning 0,
> I think most errors will still go unnoticed here, since most callers of
> do_subchannel_work() seem to ignore the return value ... so I wonder
> whether we rather want to have another way to recognize this condition.
> If the comment is right and this really can not happen, I think you
> should use an g_assert_notreached() here instead. Otherwise the comment
> should be changed and it's maybe a good idea to use a
> qemu_log_mask(LOG_GUEST_ERROR, "subchannel in bad state bla bla...") here.

OK, after reading through patch 4/9 I think I've got the basic idea now
... you'll eventually set sch->iret.cc = 3 here instead, so the exact
error code does not really matter here.
But still - if it "Cannot happen", maybe an assert() or an additional
qemu_log would be appropriate?

 Thomas