On 09/12/2017 04:37 PM, Cornelia Huck wrote: > On Mon, 11 Sep 2017 13:36:29 +0200 > Halil Pasic <[email protected]> wrote: > >> On 09/11/2017 12:07 PM, Cornelia Huck wrote: >>> On Fri, 8 Sep 2017 17:24:46 +0200 >>> Halil Pasic <[email protected]> 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. >> >>> >>> 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. >> >>> (Another idea would be to have the callback prepare the scsw via helper >>> functions. We'd just keep -EAGAIN to keep processing a chain and 0 to >>> stop.) >>> >> >> That was my first idea how to improve on this. I should still have the >> code (patches), but I'm not sure whether it's clean or lumped together >> with other experiments. >> >> After pushing the handling down the call chain (caller would use >> inline functions to manipulate SCSW), I've realized that it does >> not buy us much/anything expect the better names, while we get >> the machine code manipulating the SCSW generated in multiple >> instead of in one place. I also showed the results to Dong Jia and >> he was ambivalent too: said something like it does look better, >> but it ain't better enough to make it worthwhile. >> >> This is why I've decided to go with a less intrusive approach: >> just change the names so that it's obvious what's happening. > > Something like return channel_program_check(...); or so would be quite > obvious, I think. > > But yes, it will be easier to evaluate this for an actual patch ;) > OK, I will look into this, and probably send an RFC these days. >>>> 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? > > Reword the sentence to use 'short data with incorrect length checking > suppressed' or so? > Could you provide a whole sentence? I think my original sentence is OK (purpose: indicate that virtio is special, and that we have to bend the architecture a bit), but I agree, being a little more verbose may be a good idea. I just can't come up with a nice sentence. Halil
