diff mbox series

hw/scsi/vmw_pvscsi.c: Fix wrong usage of gboolean types in PVSCSIState

Message ID 20200913234451.170022-1-ameynarkhede03@gmail.com
State New
Headers show
Series hw/scsi/vmw_pvscsi.c: Fix wrong usage of gboolean types in PVSCSIState | expand

Commit Message

Amey Narkhede Sept. 13, 2020, 11:44 p.m. UTC
rings_info_valid, msg_ring_info_valid and use_msg fields of struct
PVSCSIState are using gboolean TRUE/FALSE values with the type uint8_t.
Change their type to bool along with the usage of initialization macro
VMSTATE_BOOL during initialization of vmstate_pvscsi and
pvscsi_properties.

Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
---
 hw/scsi/vmw_pvscsi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--
2.28.0

This is my first qemu patch. Let know if there are any mistakes

Comments

Philippe Mathieu-Daudé Sept. 14, 2020, 6:32 a.m. UTC | #1
Hi Amey,

On 9/14/20 1:44 AM, Amey Narkhede wrote:
> rings_info_valid, msg_ring_info_valid and use_msg fields of struct
> PVSCSIState are using gboolean TRUE/FALSE values with the type uint8_t.
> Change their type to bool along with the usage of initialization macro
> VMSTATE_BOOL during initialization of vmstate_pvscsi and
> pvscsi_properties.
> 
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> ---
>  hw/scsi/vmw_pvscsi.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c071e0c7aa..86f00e3d7e 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -123,9 +123,9 @@ typedef struct {
>      /* Collector for current command data */
>      uint32_t curr_cmd_data[PVSCSI_MAX_CMD_DATA_WORDS];
> 
> -    uint8_t rings_info_valid;            /* Whether data rings initialized   */
> -    uint8_t msg_ring_info_valid;         /* Whether message ring initialized */
> -    uint8_t use_msg;                     /* Whether to use message ring      */
> +    bool rings_info_valid;            /* Whether data rings initialized   */
> +    bool msg_ring_info_valid;         /* Whether message ring initialized */
> +    bool use_msg;                     /* Whether to use message ring      */
> 
>      uint8_t msi_used;                    /* For migration compatibility      */
>      PVSCSIRingInfo rings;                /* Data transfer rings manager      */
> @@ -349,8 +349,8 @@ pvscsi_reset_state(PVSCSIState *s)
>      s->reg_command_status = PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>      s->reg_interrupt_status = 0;
>      pvscsi_ring_cleanup(&s->rings);
> -    s->rings_info_valid = FALSE;
> -    s->msg_ring_info_valid = FALSE;
> +    s->rings_info_valid = false;
> +    s->msg_ring_info_valid = false;
>      QTAILQ_INIT(&s->pending_queue);
>      QTAILQ_INIT(&s->completion_queue);
>  }
> @@ -792,7 +792,7 @@ pvscsi_on_cmd_setup_rings(PVSCSIState *s)
>      pvscsi_dbg_dump_tx_rings_config(rc);
>      pvscsi_ring_init_data(&s->rings, rc);
> 
> -    s->rings_info_valid = TRUE;
> +    s->rings_info_valid = true;
>      return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>  }
> 
> @@ -874,7 +874,7 @@ pvscsi_on_cmd_setup_msg_ring(PVSCSIState *s)
>          if (pvscsi_ring_init_msg(&s->rings, rc) < 0) {
>              return PVSCSI_COMMAND_PROCESSING_FAILED;
>          }
> -        s->msg_ring_info_valid = TRUE;
> +        s->msg_ring_info_valid = true;
>      }
>      return sizeof(PVSCSICmdDescSetupMsgRing) / sizeof(uint32_t);
>  }
> @@ -1232,9 +1232,9 @@ static const VMStateDescription vmstate_pvscsi = {
>          VMSTATE_UINT32(curr_cmd_data_cntr, PVSCSIState),
>          VMSTATE_UINT32_ARRAY(curr_cmd_data, PVSCSIState,
>                               ARRAY_SIZE(((PVSCSIState *)NULL)->curr_cmd_data)),
> -        VMSTATE_UINT8(rings_info_valid, PVSCSIState),
> -        VMSTATE_UINT8(msg_ring_info_valid, PVSCSIState),
> -        VMSTATE_UINT8(use_msg, PVSCSIState),
> +        VMSTATE_BOOL(rings_info_valid, PVSCSIState),
> +        VMSTATE_BOOL(msg_ring_info_valid, PVSCSIState),
> +        VMSTATE_BOOL(use_msg, PVSCSIState),

I believe this change the migration data structure. This
area is described in "Changing migration data structures"
in docs/devel/migration.rst.

If this structure were not affected, your change would be
a good cleanup. However changing migration can become a
nightmare, so ... cleaning this is hard.

Cc'ing Dave (a migration maintainer) as I'm not sure there
already is a document describing easily this problem.

> 
>          VMSTATE_UINT64(rings.rs_pa, PVSCSIState),
>          VMSTATE_UINT32(rings.txr_len_mask, PVSCSIState),
> @@ -1255,7 +1255,7 @@ static const VMStateDescription vmstate_pvscsi = {
>  };
> 
>  static Property pvscsi_properties[] = {
> -    DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
> +    DEFINE_PROP_BOOL("use_msg", PVSCSIState, use_msg, true),
>      DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
>                      PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
>      DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,
> --
> 2.28.0
> 
> This is my first qemu patch. Let know if there are any mistakes
>
Dr. David Alan Gilbert Sept. 15, 2020, 7 p.m. UTC | #2
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Hi Amey,
> 
> On 9/14/20 1:44 AM, Amey Narkhede wrote:
> > rings_info_valid, msg_ring_info_valid and use_msg fields of struct
> > PVSCSIState are using gboolean TRUE/FALSE values with the type uint8_t.
> > Change their type to bool along with the usage of initialization macro
> > VMSTATE_BOOL during initialization of vmstate_pvscsi and
> > pvscsi_properties.
> > 
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > ---
> >  hw/scsi/vmw_pvscsi.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> > index c071e0c7aa..86f00e3d7e 100644
> > --- a/hw/scsi/vmw_pvscsi.c
> > +++ b/hw/scsi/vmw_pvscsi.c
> > @@ -123,9 +123,9 @@ typedef struct {
> >      /* Collector for current command data */
> >      uint32_t curr_cmd_data[PVSCSI_MAX_CMD_DATA_WORDS];
> > 
> > -    uint8_t rings_info_valid;            /* Whether data rings initialized   */
> > -    uint8_t msg_ring_info_valid;         /* Whether message ring initialized */
> > -    uint8_t use_msg;                     /* Whether to use message ring      */
> > +    bool rings_info_valid;            /* Whether data rings initialized   */
> > +    bool msg_ring_info_valid;         /* Whether message ring initialized */
> > +    bool use_msg;                     /* Whether to use message ring      */
> > 
> >      uint8_t msi_used;                    /* For migration compatibility      */
> >      PVSCSIRingInfo rings;                /* Data transfer rings manager      */
> > @@ -349,8 +349,8 @@ pvscsi_reset_state(PVSCSIState *s)
> >      s->reg_command_status = PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
> >      s->reg_interrupt_status = 0;
> >      pvscsi_ring_cleanup(&s->rings);
> > -    s->rings_info_valid = FALSE;
> > -    s->msg_ring_info_valid = FALSE;
> > +    s->rings_info_valid = false;
> > +    s->msg_ring_info_valid = false;
> >      QTAILQ_INIT(&s->pending_queue);
> >      QTAILQ_INIT(&s->completion_queue);
> >  }
> > @@ -792,7 +792,7 @@ pvscsi_on_cmd_setup_rings(PVSCSIState *s)
> >      pvscsi_dbg_dump_tx_rings_config(rc);
> >      pvscsi_ring_init_data(&s->rings, rc);
> > 
> > -    s->rings_info_valid = TRUE;
> > +    s->rings_info_valid = true;
> >      return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
> >  }
> > 
> > @@ -874,7 +874,7 @@ pvscsi_on_cmd_setup_msg_ring(PVSCSIState *s)
> >          if (pvscsi_ring_init_msg(&s->rings, rc) < 0) {
> >              return PVSCSI_COMMAND_PROCESSING_FAILED;
> >          }
> > -        s->msg_ring_info_valid = TRUE;
> > +        s->msg_ring_info_valid = true;
> >      }
> >      return sizeof(PVSCSICmdDescSetupMsgRing) / sizeof(uint32_t);
> >  }
> > @@ -1232,9 +1232,9 @@ static const VMStateDescription vmstate_pvscsi = {
> >          VMSTATE_UINT32(curr_cmd_data_cntr, PVSCSIState),
> >          VMSTATE_UINT32_ARRAY(curr_cmd_data, PVSCSIState,
> >                               ARRAY_SIZE(((PVSCSIState *)NULL)->curr_cmd_data)),
> > -        VMSTATE_UINT8(rings_info_valid, PVSCSIState),
> > -        VMSTATE_UINT8(msg_ring_info_valid, PVSCSIState),
> > -        VMSTATE_UINT8(use_msg, PVSCSIState),
> > +        VMSTATE_BOOL(rings_info_valid, PVSCSIState),
> > +        VMSTATE_BOOL(msg_ring_info_valid, PVSCSIState),
> > +        VMSTATE_BOOL(use_msg, PVSCSIState),
> 
> I believe this change the migration data structure. This
> area is described in "Changing migration data structures"
> in docs/devel/migration.rst.
> 
> If this structure were not affected, your change would be
> a good cleanup. However changing migration can become a
> nightmare, so ... cleaning this is hard.
> 
> Cc'ing Dave (a migration maintainer) as I'm not sure there
> already is a document describing easily this problem.

Well; the migration structures are...very unstructured.
It turns out that VMSTATE_BOOL already reads/writes a raw
byte to the file - so it's the same size as the VMSTATE_UINT8 at least.
Disappointingly get_bool and put_bool in vmstate-types.c don't do any
sanitation either; so you get whatever the raw value is.

glib's FALSE/TRUE is:

#define FALSE   (0)
#define TRUE    (!FALSE)

so that's probably fine;  as long as no one does anything silly like
compare the value with TRUE or true.

Dave

> > 
> >          VMSTATE_UINT64(rings.rs_pa, PVSCSIState),
> >          VMSTATE_UINT32(rings.txr_len_mask, PVSCSIState),
> > @@ -1255,7 +1255,7 @@ static const VMStateDescription vmstate_pvscsi = {
> >  };
> > 
> >  static Property pvscsi_properties[] = {
> > -    DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
> > +    DEFINE_PROP_BOOL("use_msg", PVSCSIState, use_msg, true),
> >      DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
> >                      PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
> >      DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,
> > --
> > 2.28.0
> > 
> > This is my first qemu patch. Let know if there are any mistakes
> > 
>
diff mbox series

Patch

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index c071e0c7aa..86f00e3d7e 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -123,9 +123,9 @@  typedef struct {
     /* Collector for current command data */
     uint32_t curr_cmd_data[PVSCSI_MAX_CMD_DATA_WORDS];

-    uint8_t rings_info_valid;            /* Whether data rings initialized   */
-    uint8_t msg_ring_info_valid;         /* Whether message ring initialized */
-    uint8_t use_msg;                     /* Whether to use message ring      */
+    bool rings_info_valid;            /* Whether data rings initialized   */
+    bool msg_ring_info_valid;         /* Whether message ring initialized */
+    bool use_msg;                     /* Whether to use message ring      */

     uint8_t msi_used;                    /* For migration compatibility      */
     PVSCSIRingInfo rings;                /* Data transfer rings manager      */
@@ -349,8 +349,8 @@  pvscsi_reset_state(PVSCSIState *s)
     s->reg_command_status = PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
     s->reg_interrupt_status = 0;
     pvscsi_ring_cleanup(&s->rings);
-    s->rings_info_valid = FALSE;
-    s->msg_ring_info_valid = FALSE;
+    s->rings_info_valid = false;
+    s->msg_ring_info_valid = false;
     QTAILQ_INIT(&s->pending_queue);
     QTAILQ_INIT(&s->completion_queue);
 }
@@ -792,7 +792,7 @@  pvscsi_on_cmd_setup_rings(PVSCSIState *s)
     pvscsi_dbg_dump_tx_rings_config(rc);
     pvscsi_ring_init_data(&s->rings, rc);

-    s->rings_info_valid = TRUE;
+    s->rings_info_valid = true;
     return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
 }

@@ -874,7 +874,7 @@  pvscsi_on_cmd_setup_msg_ring(PVSCSIState *s)
         if (pvscsi_ring_init_msg(&s->rings, rc) < 0) {
             return PVSCSI_COMMAND_PROCESSING_FAILED;
         }
-        s->msg_ring_info_valid = TRUE;
+        s->msg_ring_info_valid = true;
     }
     return sizeof(PVSCSICmdDescSetupMsgRing) / sizeof(uint32_t);
 }
@@ -1232,9 +1232,9 @@  static const VMStateDescription vmstate_pvscsi = {
         VMSTATE_UINT32(curr_cmd_data_cntr, PVSCSIState),
         VMSTATE_UINT32_ARRAY(curr_cmd_data, PVSCSIState,
                              ARRAY_SIZE(((PVSCSIState *)NULL)->curr_cmd_data)),
-        VMSTATE_UINT8(rings_info_valid, PVSCSIState),
-        VMSTATE_UINT8(msg_ring_info_valid, PVSCSIState),
-        VMSTATE_UINT8(use_msg, PVSCSIState),
+        VMSTATE_BOOL(rings_info_valid, PVSCSIState),
+        VMSTATE_BOOL(msg_ring_info_valid, PVSCSIState),
+        VMSTATE_BOOL(use_msg, PVSCSIState),

         VMSTATE_UINT64(rings.rs_pa, PVSCSIState),
         VMSTATE_UINT32(rings.txr_len_mask, PVSCSIState),
@@ -1255,7 +1255,7 @@  static const VMStateDescription vmstate_pvscsi = {
 };

 static Property pvscsi_properties[] = {
-    DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
+    DEFINE_PROP_BOOL("use_msg", PVSCSIState, use_msg, true),
     DEFINE_PROP_BIT("x-old-pci-configuration", PVSCSIState, compat_flags,
                     PVSCSI_COMPAT_OLD_PCI_CONFIGURATION_BIT, false),
     DEFINE_PROP_BIT("x-disable-pcie", PVSCSIState, compat_flags,