Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 12 Sep 2017 17:43:03 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/12/2017 04:37 PM, Cornelia Huck wrote:
> > On Mon, 11 Sep 2017 13:36:29 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/11/2017 12:07 PM, Cornelia Huck wrote:  
> >>> On Fri,  8 Sep 2017 17:24:46 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> We report incorrect length via SCSW program check instead of incorrect
> >>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> >>>> is no fitting errno for incorrect length, and since I don't like what we
> >>>> do with the errno's, as part of the fix, errnos used for control flow in
> >>>> ccw interpretation are replaced with an enum using more speaking names.    
> >>>
> >>> I'm not sure whether this is the way to go. I mainly dislike the size
> >>> of the patch (and the fact that it mixes a fix and a change of function
> >>> signature).    
> >>
> >> Do you agree that we should move away from POSIX errno codes? I think
> >> if we do, this cant' get much smaller.  
> > 
> > I'm not really a fan of defining our own return values, tbh.
> >   
> 
> I've suspected. But your statement, although being useful, does
> not answer my question. I think we need to agree on this question
> before proceeding.
> 
> In my opinion both the EIO bug and this bug are great examples
> why the POSIX errno codes are sub-optimal and misleading, but
> that's my opinion.

It depends. I prefer them over home-grown ones.

(And I tend to dislike absolute statements.)

> 
> >>  
> >>>
> >>> Can we instead choose a mapping for incorrect length, and defer a
> >>> possible rework?
> >>>     
> >>
> >> In the commit message, I say that I don't have a fitting errno.
> >> If you tell me which one to use, I would be glad to split this up.
> >> I don't like mixing re-factoring and changing behavior myself.
> >>
> >> Can I have your position on the re-factoring (that is let us
> >> imagine I did not change handling for incorrect length)?  
> > 
> > If there is no return code that can be made to fit, we probably won't
> > be able to get around some kind of refactoring... but then I'd prefer
> > to do the refactoring first and the fix second.
> >   
> 
> That is a can do. I dislike refactoring known bugs, because fixing
> bugs is usually higher priority than making the code nicer, or even
> marginally faster. (Btw I found these while trying to refactor.)
> This however is a weak principle of mine and can be easily overpowered
> by a maintainer request for example.

If a good fix requires refactoring, I'd prefer to do the refactoring
first. I'd prefer an ugly fix first only for serious issues (and I
don't think that one counts as one.)

> >>>> For virtio, if incorrect length checking is suppressed we keep the
> >>>> current behavior (channel-program check).    
> >>>
> >>> Confused. If it is suppressed, there should not be an error, no?    
> >>
> >> No.
> >>
> >> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
> >>
> >> "If a driver did suppress length checks for a channel command, the device
> >> MUST present a check condition if the transmitted data does not contain
> >> enough data to process the command."
> >> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
> >>
> >> So for virtio we have to present a check condition. Architecturally it
> >> might look better if the one refusing is the device and not the CSS, but
> >> for that we would have to change the VIRTIO spec. With the given
> >> constraints a program check is IMHO the best fit.  
> > 
> > Ah, but that's not general length checking for virtio-ccw :)  
> 
> What is general length checking for virtio-ccw? Did I say it
> was general length checking for virtio-ccw?

Hm? Generally, suppressing is supposed to allow incorrect length
specifications. For virtio-ccw, that only applies to 'too much' and not
'not enough'.

Also, reading the statement in the spec: It only talks about a 'check
condition', not _which_ one - so there's no requirement to keep a
channel-program check (other than possibly confusing guests)?