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

[Denis V. Lunev]

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