Re: [Qemu-devel] [PATCH v2 1/9] drivers/hv: replace enum hv_message_type by u32

["Denis V. Lunev" <den@openvz.org>]

On 12/04/2015 08:38 PM, Paolo Bonzini wrote:
>
> On 04/12/2015 17:55, Denis V. Lunev wrote:
>> On 12/04/2015 05:41 PM, Paolo Bonzini wrote:
>>> On 04/12/2015 15:33, Denis V. Lunev wrote:
>>>> On 12/02/2015 03:22 PM, Paolo Bonzini wrote:
>>>>> On 30/11/2015 17:22, Andrey Smetanin wrote:
>>>>>> enum hv_message_type inside struct hv_message, hv_post_message
>>>>>> is not size portable. Replace enum by u32.
>>>>> It's only non-portable inside structs.  Okay to apply just these:
>>>>>
>>>>> @@ -172,7 +174,7 @@ union hv_message_flags {
>>>>>
>>>>>     /* Define synthetic interrupt controller message header. */
>>>>>     struct hv_message_header {
>>>>> -    u32 message_type;
>>>>> +    enum hv_message_type message_type;
>>>>>         u8 payload_size;
>>>>>         union hv_message_flags message_flags;
>>>>>         u8 reserved[2];
>>>>> @@ -345,7 +347,7 @@ enum hv_call_code {
>>>>>     struct hv_input_post_message {
>>>>>         union hv_connection_id connectionid;
>>>>>         u32 reserved;
>>>>> -    u32 message_type;
>>>>> +    enum hv_message_type message_type;
>>>>>         u32 payload_size;
>>>>>         u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
>>>>>     };
>>>>>
>>>>> ?
>>>>>
>>>>> Paolo
>>>> sorry for the delay.
>>>>
>>>> Andrey is on vacation till the end of the week.
>>>>
>>>> This could be not enough for some compilers as this exact
>>>> enum could be signed and unsigned depends upon the
>>>> implementation of the compiler and if it is signed we
>>>> can face signed/unsigned comparison in ifs.
>>> But why is that a problem?  The issue is pre-existing anyway; the only
>>> one that can cause bugs when moving code to uapi/ (i.e. which means it
>>> can be used on non-x86 platforms) is the size of the enum, not the
>>> signedness.
>> we are now comparing enum with enum which are the same type.
>> With the change you are proposing we will compare enum
>> with u32 which are different.
> This is only an issue in C++.
>
>> Original suggestion from Andrey was safe in this respect.
> Sure, but it makes code less clear.
>
> Paolo

ok, this seems reasonable. Why not to reduce the patch :)
We'll send an update on Monday.

Are there any other issue with the patchset?

Den