* Re: [bug report] vfio: ccw: introduce a finite state machine
[not found] <20170404120223.GA8525@mwanda>
@ 2017-04-04 12:40 ` Cornelia Huck
0 siblings, 0 replies; only message in thread
From: Cornelia Huck @ 2017-04-04 12:40 UTC (permalink / raw)
To: linux-s390
On Tue, 4 Apr 2017 15:02:24 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Dong Jia Shi,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch bbe37e4cb897: "vfio: ccw: introduce a finite state machine"
> from Mar 17, 2017, leads to the following Smatch complaint:
>
> drivers/s390/cio/vfio_ccw_ops.c:151 vfio_ccw_mdev_remove()
> error: we previously assumed 'private' could be null (see line 137)
>
> drivers/s390/cio/vfio_ccw_ops.c
> 131 static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
> 132 {
> 133 struct vfio_ccw_private *private =
> 134 dev_get_drvdata(mdev_parent_dev(mdev));
> 135 int ret;
> 136
> 137 if (!private)
> ^^^^^^^^
> New check for NULL.
>
> 138 goto out;
>
> I always feel like "out" is a crap name for a label because it doesn't
> give you any clue what the goto does. Label names should be a verb
> goto inc_available. I see this a lot in reviewing static checker
> warnings that out labels are more buggy than verb based labels.
'out' can be a good label name in certain situations, but it seems
there's some more confusion happening here anyway, so let's focus on
that.
I'm not quite clear when private can actually be NULL, as code grabbing
the driver data of the mdev device sometimes checks for it and
sometimes doesn't.
>
> 139
> 140 if ((private->state == VFIO_CCW_STATE_NOT_OPER) ||
> 141 (private->state == VFIO_CCW_STATE_STANDBY))
> 142 goto out;
It would be more readable to reverse that condition, I think.
> 143
> 144 ret = vfio_ccw_mdev_reset(mdev);
> 145 if (ret)
> 146 return ret;
Is the exit actually correct here? Don't we need to set an error state
or something?
> 147
> 148 private->state = VFIO_CCW_STATE_STANDBY;
> 149
> 150 out:
> 151 private->mdev = NULL;
> ^^^^^^^^^^^^^^^^^^^^^
> New dereference.
>
> 152 atomic_inc(&private->avail);
> 153
^ permalink raw reply [flat|nested] only message in thread