Re: [Xen-devel] [PATCH v2] CODING_STYLE: document intended usage of types

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 8/5/19 2:19 PM, Jan Beulich wrote:
> On 05.08.2019 15:01, George Dunlap wrote:
>> On 8/5/19 12:55 PM, Jan Beulich wrote:
>>> On 05.08.2019 12:58, George Dunlap wrote:
>>>> On 11/26/18 9:31 AM, Jan Beulich wrote:
>>>>> +Fixed width types should only be used when a fixed width quantity is
>>>>> +meant (which for example may be a value read from or to be written to a
>>>>> +register).
>>>>
>>>> I'm having trouble understanding the intent / implications of this one.
>>>>    Can you give me an example of where you've seen a fixed width type used
>>>> inappropriately?
>>>
>>> Grep the code base for "uint32_t size" for example. These should
>>> (almost?) all be unsigned int (or, where necessary, size_t).
>>
>> Inside the hypervisor codebase anyway, I see these patterns for
>> `uint32_t size`:
>>
>> 1. Inside tracing structures, where the code may be used either by
>> 32-bit or 64-bit userspace tools
> 
> I simply assume in these cases use of fixed width types is
> intended.
> 
>> 2. Inside headers for public interfaces (same reason).
> 
> Here fixed width types are definitely the right choice.
> 
>> 3. In function signatures for emulation code.  I assume this is because
>> sizes are architecturally defined.
> 
> Taking null_read() as an example - no, there's no need for a fixed
> width type here. Even if the value was read from a register,
> propagating the value still only needs to guarantee no truncation.
> But the value can't come from a register directly anyway, or else
> the type would need to be uint64_t. The type "addr" is wrongly
> using uint64_t here, too, in my opinion: It should be unsigned long
> or paddr_t, depending on whether we're talking of linear or physical
> addresses (I think it's the latter here).
> 
>> 4. Inside decompression code, to interface with public sizes.
> 
> I don't think there's any interfacing with "public" structures
> there.
> 
> 5. sysctl_cov_op() should again use unsigned int
> 6. struct elf_sym_header too should use unsigned int
> 7. struct hvm_domain_context may want to continue to use uint8_t
>     (albeit unsigned char would be quite fine as well), but its
>     two uint32_t uses could once again be unsigned int
> etc

OK, well this gives me an idea what you mean.  I'm OK with this; but it
would probably be good if one of the ARM guys Acked it as well.

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel