On Tue, 12 Sep 2017 17:43:03 +0200 Halil Pasic <pa...@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 <pa...@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 <pa...@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)?