Re: [Qemu-devel] [PATCH 2/5] HACKING: add C type rules

[Blue Swirl]

On Sun, Aug 15, 2010 at 9:37 PM, Anthony Liguori <address@hidden> wrote:
> Hi Blue,
>
> Thanks for putting this document together.  It should be quiet helpful!
>
> On 08/15/2010 12:50 PM, Blue Swirl wrote:
>>
>> Add C type rules, adapted from libvirt HACKING. Also include
>> a description of special QEMU scalar types.
>>
>> Move typedef rule from CODING_STYLE rule 3 to HACKING rule 6
>> where it belongs.
>>
>> Signed-off-by: Blue Swirl<address@hidden>
>> ---
>>  CODING_STYLE |    3 --
>>  HACKING      |   64
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/CODING_STYLE b/CODING_STYLE
>> index 92036f3..2c8268d 100644
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -46,9 +46,6 @@ names are
>> lower_case_with_underscores_ending_with_a_t, like the POSIX
>>  uint64_t and family.  Note that this last convention contradicts POSIX
>>  and is therefore likely to be changed.
>>
>> -Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>> -QEMU coding style.
>> -
>>  When wrapping standard library functions, use the prefix qemu_ to alert
>>  readers that they are seeing a wrapped version; otherwise avoid this
>> prefix.
>>
>> diff --git a/HACKING b/HACKING
>> index e60aa48..7c6b49e 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -4,3 +4,67 @@ For variadic macros, stick with C99 syntax:
>>
>>  #define DPRINTF(fmt, ...)                                       \
>>      do { printf("IRQ: " fmt, ## __VA_ARGS__); } while (0)
>> +
>> +2. C types
>> +
>> +It should be common sense to use the right type, but we have collected
>> +a few useful guidelines here.
>> +
>> +2.1. Scalars
>> +
>> +If you're using "int" or "long", odds are good that there's a better
>> type.
>> +If a variable is counting something, it should be declared with an
>> +unsigned type.
>> +
>> +If it's host memory-size related, size_t should be a good choice (use
>> +ssize_t only if required). Guest RAM memory offsets must use ram_addr_t,
>> +but only for RAM, it may not cover whole guest address space.
>>
>
> This needs a little more explanation.  There are two different address
> spaces.  target_phys_addr_t represents guest RAM physical addresses.

Not only RAM, it can be MMIO.

>  ram_addr_t is a QEMU internal address space that maps guest RAM physical
> addresses into an intermediate address space that can map to host virtual
> address spaces.
>
> Generally speaking, the size of guest memory can always fit into ram_addr_t
> but it would not be correct to store an actual guest physical address in a
> ram_addr_t.

I'll add this to the next version.

>
>> +If it's file-size related, use off_t.
>> +If it's file-offset related (i.e., signed), use off_t.
>> +If it's just counting small numbers use "unsigned int";
>> +(on all but oddball embedded systems, you can assume that that
>> +type is at least four bytes wide).
>> +
>> +In the event that you require a specific width, use a standard type
>> +like int32_t, uint32_t, uint64_t, etc.  The specific types are
>> +mandatory for VMState fields.
>> +
>> +Don't use Linux kernel internal types like u32, __u32 or __le32.
>> +
>> +Use target_phys_addr_t for hardware physical addresses except pcibus_t
>> +for PCI addresses.  Use target_ulong (or abi_ulong) for CPU
>> +virtual addresses, however devices should not need to use target_ulong.
>> +
>> +While using "bool" is good for readability, it comes with minor caveats:
>> + - Don't use "bool" in places where the type size must be constant across
>> +   all systems, like public interfaces and on-the-wire protocols.
>> + - Don't compare a bool variable against the literal, "true",
>> +   since a value with a logical non-false value need not be "1".
>> +   I.e., don't write "if (seen == true) ...".  Rather, write "if
>> (seen)...".
>> +
>> +Of course, take all of the above with a grain of salt.  If you're about
>> +to use some system interface that requires a type like size_t, pid_t or
>> +off_t, use matching types for any corresponding variables.
>> +
>> +Also, if you try to use e.g., "unsigned int" as a type, and that
>> +conflicts with the signedness of a related variable, sometimes
>> +it's best just to use the *wrong* type, if "pulling the thread"
>> +and fixing all related variables would be too invasive.
>> +
>> +Finally, while using descriptive types is important, be careful not to
>> +go overboard.  If whatever you're doing causes warnings, or requires
>> +casts, then reconsider or ask for help.
>> +
>> +2.2. Pointers
>> +
>> +Ensure that all of your pointers are "const-correct".
>> +Unless a pointer is used to modify the pointed-to storage,
>> +give it the "const" attribute.  That way, the reader knows
>> +up-front that this is a read-only pointer.  Perhaps more
>> +importantly, if we're diligent about this, when you see a non-const
>> +pointer, you're guaranteed that it is used to modify the storage
>> +it points to, or it is aliased to another pointer that is.
>> +
>> +2.3. Typedefs
>> +Typedefs are used to eliminate the redundant 'struct' keyword.
>>
>
> It's worth mention the reserved namespaces in C.  That is, underscore
> capital, double underscore, and underscore 't' suffixes should be avoid.

OK, thanks for the review.