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

[Thomas Huth]

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.

 Thomas