vfio: ccw: introduce a finite state machine
A message that is not in the archive (sent off-list or not yet received): 20170404120223.GA8525@mwanda
Quoted 19 lines · On Tue, 4 Apr 2017 15:02:24 +0300 Dan Carpenter <[email protected]>…
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:151vfio_ccw_mdev_remove()
error: we previously assumed 'private' could be null (see line 137)
drivers/s390/cio/vfio_ccw_ops.c
131 static intvfio_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) ^^^^^^^^
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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?
Quoted 10 lines
147 148 private->state = VFIO_CCW_STATE_STANDBY; 149 150 out: 151 private->mdev = NULL; ^^^^^^^^^^^^^^^^^^^^^New dereference.
152 atomic_inc(&private->avail); 153