Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control

[Halil Pasic <pasic@linux.vnet.ibm.com>]

[..]
>>
>> Yeah, the ABI is smart enough (where it matters) and this one is obviously
>> less that 8 bytes. I read this as you  assumed that the return won't be
>> passed via register (general purpose register 2 for a z host + ELF assumed),
>> and that would have been ugly indeed.
>>
>> Btw I have seen putting an integral into a struct for type checking
>> in the linux kernel too. For instance from:
>> arch/s390/include/asm/page.h
>>
>> """
>> /*
>>  * These are used to make use of C type-checking..
>>  */
>>
>> typedef struct { unsigned long pgprot; } pgprot_t;
>> typedef struct { unsigned long pgste; } pgste_t;
>> typedef struct { unsigned long pte; } pte_t;
>> typedef struct { unsigned long pmd; } pmd_t;
>> typedef struct { unsigned long pud; } pud_t;
>> typedef struct { unsigned long pgd; } pgd_t;
>> typedef pte_t *pgtable_t;
>>
>> #define pgprot_val(x)   ((x).pgprot)
>> #define pgste_val(x)    ((x).pgste)
>> #define pte_val(x)      ((x).pte)
>> #define pmd_val(x)      ((x).pmd)
>> #define pud_val(x)      ((x).pud)
>> #define pgd_val(x)      ((x).pgd)
>>
>> #define __pgste(x)      ((pgste_t) { (x) } )
>> #define __pte(x)        ((pte_t) { (x) } )
>> #define __pmd(x)        ((pmd_t) { (x) } )
>> #define __pud(x)        ((pud_t) { (x) } )
>> #define __pgd(x)        ((pgd_t) { (x) } )
>> #define __pgprot(x)     ((pgprot_t) { (x) } )
>> """
>>
>> If you think, I could add a similar comment indicating that my
>> struct is also just for type-checking.
> 
> No, I think you've got the reason here slightly wrong. The kernel only
> uses this since the pte_t and friends need to be urgently portable
> between the different architectures. Have a look at the kernel
> Documentation/CodingStyle file, it explicitly mentions this in chapter 5
> ("Typedefs").
>

Disclaimer: I agree with your conclusion, the maintainers will have
to make the final call here. I will still reply point by point for
the fun of pointless academic discussions.

IMHO we don't talk about typedefs here but about type checking. Btw QEMU
has the exact opposite policy regarding typedefs and structs than Linux.
You want to say that the comment at the top of my quote is wrong, and
that it should be rather "These are used for hiding representation.."
that "These are used to make use of C type-checking.."?

I'm also curious which of the rules would your original suggestion of
doing "typedef unsigned int IOInstEnding" match (from chapter 5
"Typedefs")? ;)

 
>>> Then, in the follow up patches, you do something like this:
>>>
>>>    return (IOInstEnding){.cc = 0};
>>>
>>> ... and that just looks very, very ugly in my eyes. The more I look at
>>
>> Interesting, I found this quite expressive.
> 
> C'mon, we're writing C code, not Java ;-)

Yeah, me written much C++ and also Java before might be a part
of the problem. Btw there is no such construct in Java AFAIR :P.
> 
>>> it, the more I think we really want to have a named enum instead. That
>>> will give you some sort of basic type safety and semantics, too, and
>>
>> AFAIK we don't have strongly typed enums in C, at least not from
>> the language perspective. In c++ we got enum class and enum struct
>> with c++11 for that reason.
>>
>>> we'll also get proper names for those magic values - otherwise I'll
>>> always have to look up what cc = 2 or cc = 3 means... (I always keep
>>> forgetting what each value means...)
>>
>> Can you suggest some proper names for those magic values? Unfortunately
>> the PoP refers to this stuff as setting condition code [0-3], so in my
>> reading the numbers are the canonical names for this stuff. The best
>> 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3.
> 
> Well, you already gave a description in your comment in the  struct
> IOInstEnding, so maybe something similar? Or maybe this could even be
> merged with the definitions for the SIGP status codes:
> 
> #define SIGP_CC_ORDER_CODE_ACCEPTED 0
> #define SIGP_CC_STATUS_STORED       1
> #define SIGP_CC_BUSY                2
> #define SIGP_CC_NOT_OPERATIONAL     3
> 
> ?
> 

Maybe.

>> Sorry, I may be a bit to persistent on this one: I don't think it's
>> a huge difference, but I don't feel great about changing something to
>> what I think is (slightly) worse without being first convinced that
>> I was wrong.
> 
> In the end, the code has to be accepted by the maintainers, so let's
> leave the decision up to them whether they like this typedef struct
> IOInstEnding or not...

I agree. Especially if the change in behavior in #3 does not get trough
I will have to revert indicating exceptions too (like in v1), and then
I really need the struct (which can be still less than 8 bytes).

I assume you have seen my reply to Connie about the benefit: among others
it prevents using something like -EFAULT as a cc by accident which neither
an enum nor a typedef does.

In practice, I think either of the proposed solutions will do.

Halil
 
> 
>  Thomas
>