qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Huth
Subject: Re: [Qemu-devel] [PATCH v2 2/8] s390x/css: IO instr handler ending control
Date: Thu, 12 Oct 2017 08:58:45 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

On 10.10.2017 13:41, Halil Pasic wrote:
> [..]
>>>
>>> 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").
> 
> 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 certainly did not want to say that you should change the comment. I
just wanted to point you to the fact that these typedefs in the kernel
are likely rather used for hiding representation indeed, and not so much
for type checking, so using them as an example for introducing something
like type checking here in QEMU is quite a bad example.

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

None, of course. That's the difference between the kernel and QEMU -
though I have to say that I rather prefer the kernel philosophy nowadays.

So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
think the best match for QEMU would be a

typedef enum IOInstEnding {
    CC_...,
    CC_...,
    CC_...,
    CC_...
} IOInstEnding;

You then also should at least get some compiler checking thanks to
-Wswitch and -Wenum-compare, shouldn't you?

> 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.

A confused developer could still do something like "return
(IOInstEnding){.cc = -EFAULT};", so this is also not completely safe.

 Thomas



reply via email to

[Prev in Thread] Current Thread [Next in Thread]