On 10/09/2017 01:09 PM, Cornelia Huck wrote:
> On Mon, 9 Oct 2017 12:54:03 +0200
> Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
>
>> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>>> On 04.10.2017 17:41, Halil Pasic wrote:
>
>>>> +/* IO instructions conclude according this */
>>>> +typedef struct IOInstEnding {
>>>> + /*
>>>> + * General semantic of cc codes of IO instructions is (brief):
>>>> + * 0 -- produced expected result
>>>> + * 1 -- status conditions were present or produced alternate
>>>> result
>>>> + * 2 -- ineffective, because busy with previously initiated
>>>> function
>>>> + * 3 -- ineffective, not operational
>>>> + */
>>>> + int cc;
>>>> +} IOInstEnding;
>>>
>>> Why do you need a struct for this? Do you plan to extend it later? If
>>> so, I think you should mention that in the patch description. If not,
>>> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
>>>
>>> Thomas
>>
>> We may, we may not. In the previous version we also had to support
>> do end a certain instruction with an addressing exception, but this
>> is going away in patch #3. Honestly I don't expect this being extended.
>>
>> I have other reasons for the struct. Type safety and clear semantics,
>> and frankly at least for s390 and linux I don't see any downsides given
>> what is written in the "zSeries ELF Application Binary Interface Supplement".
>> Can you please explain to me what is the problem with using this struct, and
>> what is the benefit switching to a unsigned int?
>
> Honestly, I fail to see the benefit of using a struct here... it's just
Type safety. For instance had I forgotten let's say a return -ENODEV
somewhere, my version would not compile. On the contrary with an enum (like
Thomas has proposed) it compiles just fine with my setup -- I did not try
what would happen if we call setcc(cpu, -ENODEV)
> a condition code, and while adding a comment what the various codes
Right, but it's a different _type_ of condition code (than the POSIX errno
codes for instance) so that's why it's modeled as a different type.
> mean for I/O instructions is a good idea, I think having to use a
> IOInstEnding struct just renders the code less readable.
It's probably a matter of taste. I find, for example
return (IOInstEnding){.cc = 1}
in this case more readable than
return -ENOSYS
because from the first one I know we have detected a condition
which we want to handle by setting cc 1 for the instruction. The
later requires me figuring out in the context of which instruction
handler am I called, go trough the whole call chain looking for
possible re-mappings (like for -EACCES) and finally examining the
switch statement of the instruction handler carefully.
Could you please tell me what exactly is difficult to read for
you (form what I've proposed).
Regards,
Halil
>
> [I haven't had time to look at the rest of the patches yet.]
>
