| Message ID | 20180321124026.70292-1-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-2.12] hw/rdma: fix clang compilation errors | expand |
On Wed, Mar 21, 2018 at 02:40:26PM +0200, Marcel Apfelbaum wrote: > Fix some enum castings and extra parentheses. > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/rdma/vmw/pvrdma_cmd.c | 5 +++-- > hw/rdma/vmw/pvrdma_main.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > index 293dfed29f..25f747a190 100644 > --- a/hw/rdma/vmw/pvrdma_cmd.c > +++ b/hw/rdma/vmw/pvrdma_cmd.c > @@ -73,7 +73,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma, > tbl_idx = 1; > addr_idx = 1; > while (addr_idx < nchunks) { > - if ((tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t)))) { > + if (tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t))) { > tbl_idx = 0; > dir_idx++; > pr_dbg("Mapping to table %d\n", dir_idx); > @@ -507,7 +507,8 @@ static int modify_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, > rsp->hdr.err = rdma_rm_modify_qp(&dev->rdma_dev_res, &dev->backend_dev, > cmd->qp_handle, cmd->attr_mask, > (union ibv_gid *)&cmd->attrs.ah_attr.grh.dgid, > - cmd->attrs.dest_qp_num, cmd->attrs.qp_state, > + (enum ibv_qp_state)cmd->attrs.dest_qp_num, This field is uint32_t and function gets uint32_t so we should be fine, it doesn't make sense to cast it to ibv_qp_state. > + (enum ibv_qp_state)cmd->attrs.qp_state, > cmd->attrs.qkey, cmd->attrs.rq_psn, > cmd->attrs.sq_psn); > > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c > index 99787812ba..0e3469287f 100644 > --- a/hw/rdma/vmw/pvrdma_main.c > +++ b/hw/rdma/vmw/pvrdma_main.c > @@ -91,7 +91,7 @@ static int init_dev_ring(PvrdmaRing *ring, struct pvrdma_ring **ring_state, > goto out_free_tbl; > } > /* RX ring is the second */ > - (struct pvrdma_ring *)(*ring_state)++; > + (*ring_state)++; > rc = pvrdma_ring_init(ring, name, pci_dev, > (struct pvrdma_ring *)*ring_state, > (num_pages - 1) * TARGET_PAGE_SIZE / > @@ -292,7 +292,8 @@ static void init_ports(PVRDMADev *dev, Error **errp) > memset(dev->rdma_dev_res.ports, 0, sizeof(dev->rdma_dev_res.ports)); > > for (i = 0; i < MAX_PORTS; i++) { > - dev->rdma_dev_res.ports[i].state = PVRDMA_PORT_DOWN; > + dev->rdma_dev_res.ports[i].state = > + (enum ibv_port_state)PVRDMA_PORT_DOWN; Suggesting to set it to IBV_PORT_DOWN and avoid this casting. > > dev->rdma_dev_res.ports[i].pkey_tbl = > g_malloc0(sizeof(*dev->rdma_dev_res.ports[i].pkey_tbl) * > -- > 2.13.5 >
On 03/21/2018 07:40 AM, Marcel Apfelbaum wrote: > Fix some enum castings and extra parentheses. > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/rdma/vmw/pvrdma_cmd.c | 5 +++-- > hw/rdma/vmw/pvrdma_main.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > index 293dfed29f..25f747a190 100644 > --- a/hw/rdma/vmw/pvrdma_cmd.c > +++ b/hw/rdma/vmw/pvrdma_cmd.c > @@ -73,7 +73,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma, > tbl_idx = 1; > addr_idx = 1; > while (addr_idx < nchunks) { > - if ((tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t)))) { > + if (tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t))) { Can't you still simplify that further to if (tbl_idx == TARGET_PAGE_SIZE / sizeof(uint64_t)) { > @@ -292,7 +292,8 @@ static void init_ports(PVRDMADev *dev, Error **errp) > memset(dev->rdma_dev_res.ports, 0, sizeof(dev->rdma_dev_res.ports)); > > for (i = 0; i < MAX_PORTS; i++) { > - dev->rdma_dev_res.ports[i].state = PVRDMA_PORT_DOWN; > + dev->rdma_dev_res.ports[i].state = > + (enum ibv_port_state)PVRDMA_PORT_DOWN; > This one looks suspicious - shouldn't you instead be using IBV_PORT_DOWN instead of having to cast? (Even if IBV_PORT_DOWN and PVRDMA_PORT_DOWN both have the value of 1 for now, the compiler warning is telling you that either one of the two enums can change independently in the future, and using a cast to shut up the compiler feels unsafe).
On 21/03/2018 15:20, Eric Blake wrote: > On 03/21/2018 07:40 AM, Marcel Apfelbaum wrote: >> Fix some enum castings and extra parentheses. >> >> Reported-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> hw/rdma/vmw/pvrdma_cmd.c | 5 +++-- >> hw/rdma/vmw/pvrdma_main.c | 5 +++-- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c >> index 293dfed29f..25f747a190 100644 >> --- a/hw/rdma/vmw/pvrdma_cmd.c >> +++ b/hw/rdma/vmw/pvrdma_cmd.c >> @@ -73,7 +73,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma, >> tbl_idx = 1; >> addr_idx = 1; >> while (addr_idx < nchunks) { >> - if ((tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t)))) { >> + if (tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t))) { > > Can't you still simplify that further to > > if (tbl_idx == TARGET_PAGE_SIZE / sizeof(uint64_t)) { > I'll try, thanks. >> @@ -292,7 +292,8 @@ static void init_ports(PVRDMADev *dev, Error **errp) >> memset(dev->rdma_dev_res.ports, 0, sizeof(dev->rdma_dev_res.ports)); >> for (i = 0; i < MAX_PORTS; i++) { >> - dev->rdma_dev_res.ports[i].state = PVRDMA_PORT_DOWN; >> + dev->rdma_dev_res.ports[i].state = >> + (enum ibv_port_state)PVRDMA_PORT_DOWN; >> > > This one looks suspicious - shouldn't you instead be using IBV_PORT_DOWN instead of having to cast? (Even if > IBV_PORT_DOWN and PVRDMA_PORT_DOWN both have the value of 1 for now, the compiler warning is telling you that either one > of the two enums can change independently in the future, and using a cast to shut up the compiler feels unsafe). > Yes, Yuval pointed it out too, I'll change it. Thanks, Marcel
diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c index 293dfed29f..25f747a190 100644 --- a/hw/rdma/vmw/pvrdma_cmd.c +++ b/hw/rdma/vmw/pvrdma_cmd.c @@ -73,7 +73,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma, tbl_idx = 1; addr_idx = 1; while (addr_idx < nchunks) { - if ((tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t)))) { + if (tbl_idx == (TARGET_PAGE_SIZE / sizeof(uint64_t))) { tbl_idx = 0; dir_idx++; pr_dbg("Mapping to table %d\n", dir_idx); @@ -507,7 +507,8 @@ static int modify_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, rsp->hdr.err = rdma_rm_modify_qp(&dev->rdma_dev_res, &dev->backend_dev, cmd->qp_handle, cmd->attr_mask, (union ibv_gid *)&cmd->attrs.ah_attr.grh.dgid, - cmd->attrs.dest_qp_num, cmd->attrs.qp_state, + (enum ibv_qp_state)cmd->attrs.dest_qp_num, + (enum ibv_qp_state)cmd->attrs.qp_state, cmd->attrs.qkey, cmd->attrs.rq_psn, cmd->attrs.sq_psn); diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c index 99787812ba..0e3469287f 100644 --- a/hw/rdma/vmw/pvrdma_main.c +++ b/hw/rdma/vmw/pvrdma_main.c @@ -91,7 +91,7 @@ static int init_dev_ring(PvrdmaRing *ring, struct pvrdma_ring **ring_state, goto out_free_tbl; } /* RX ring is the second */ - (struct pvrdma_ring *)(*ring_state)++; + (*ring_state)++; rc = pvrdma_ring_init(ring, name, pci_dev, (struct pvrdma_ring *)*ring_state, (num_pages - 1) * TARGET_PAGE_SIZE / @@ -292,7 +292,8 @@ static void init_ports(PVRDMADev *dev, Error **errp) memset(dev->rdma_dev_res.ports, 0, sizeof(dev->rdma_dev_res.ports)); for (i = 0; i < MAX_PORTS; i++) { - dev->rdma_dev_res.ports[i].state = PVRDMA_PORT_DOWN; + dev->rdma_dev_res.ports[i].state = + (enum ibv_port_state)PVRDMA_PORT_DOWN; dev->rdma_dev_res.ports[i].pkey_tbl = g_malloc0(sizeof(*dev->rdma_dev_res.ports[i].pkey_tbl) *
Fix some enum castings and extra parentheses. Reported-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/rdma/vmw/pvrdma_cmd.c | 5 +++-- hw/rdma/vmw/pvrdma_main.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)