[2/5] HACKING: add C type rules

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 <blauwirbel@gmail.com>
---
 CODING_STYLE |    3 --
 HACKING      |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

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<blauwirbel@gmail.com>
> ---
>   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.  
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.

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

Regards,

Anthony Liguori

On Sun, Aug 15, 2010 at 9:37 PM, Anthony Liguori <anthony@codemonkey.ws> 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<blauwirbel@gmail.com>
>> ---
>>  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.