Time for a code audit?
Sell, Timothy C
Timothy.Sell at unisys.com
Mon May 2 16:38:27 UTC 2016
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Monday, May 02, 2016 8:55 AM
> To: Kershner, David A
> Cc: gregkh at linuxfoundation.org; driverdev-devel at linuxdriverproject.org;
> Sell, Timothy C; *S-Par-Maintainer; Binder, David Anthony
> Subject: Re: Time for a code audit?
>
> Reviewing drivers/staging/unisys/visornic/visornic_main.c
>
> 212 static int
> 213 visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int
> firstfraglen,
> 214 unsigned int frags_max,
> 215 struct phys_info frags[])
> 216 {
> 217 unsigned int count = 0, ii, size, offset = 0, numfrags;
> ^^
>
> The second "i" character indicates that it is an integer. Which is ugly
> and, of course, misleading.
>
> 296 static ssize_t enable_ints_write(struct file *file,
> 297 const char __user *buffer,
> 298 size_t count, loff_t *ppos)
> 299 {
> 300 /*
> 301 * Don't want to break ABI here by having a debugfs
> 302 * file that no longer exists or is writable, so
> 303 * lets just make this a vestigual function
> 304 */
> 305 return count;
> 306 }
>
> We don't care about ABI so much as we do about applications or scripts.
> This was added by Neil Horman because the original code was useless and
> dangerous. It was the right patch for him, because he doesn't know
> about the userspace so he can't say if anything will break. But
> presumably you guys know. Does anything break if we remove this debugfs
> file?
>
> Also if so then shame shame shame. User space is not supposed to depend
> on debugfs being writable. Just delete this...
>
> 1019 */
> 1020 static void
> 1021 visornic_set_multi(struct net_device *netdev)
> 1022 {
> 1023 struct uiscmdrsp *cmdrsp;
> 1024 struct visornic_devdata *devdata = netdev_priv(netdev);
> 1025
> 1026 /* any filtering changes */
> 1027 if (devdata->old_flags != netdev->flags) {
> 1028 if ((netdev->flags & IFF_PROMISC) !=
> 1029 (devdata->old_flags & IFF_PROMISC)) {
>
> Reverse these if statements and pull the code in two indent levels.
>
> 1154 /**
> 1155 * visornic_rx - Handle receive packets coming back from IO Part
> 1156 * @cmdrsp: Receive packet returned from IO Part
> 1157 *
> 1158 * Got a receive packet back from the IO Part, handle it and send
> 1159 * it up the stack.
> 1160 * Returns void
>
> Actually it returns either zero or one but in a convoluted way. Update
> it to use literals and update the comment.
>
> 1361 /**
> 1362 * devdata_initialize - Initialize devdata structure
> 1363 * @devdata: visornic_devdata structure to initialize
> 1364 * #dev: visorbus_deviced it belongs to
> 1365 *
> 1366 * Setup initial values for the visornic based on channel and default
> 1367 * values.
> 1368 * Returns a pointer to the devdata if successful, else NULL
> 1369 */
> 1370 static struct visornic_devdata *
> 1371 devdata_initialize(struct visornic_devdata *devdata, struct
> visor_device *dev)
> 1372 {
> 1373 if (!devdata)
> 1374 return NULL;
>
> This check doesn't make sense and the error handling is kind of odd.
> Just remove it, remove the error handling and update the comment.
>
> 1817 err = visorbus_read_channel(dev, channel_offset,
> 1818 &devdata->num_rcv_bufs, 4);
> 1819 if (err) {
> 1820 dev_err(&dev->device,
> 1821 "%s failed to get #rcv bufs from chan (%d)\n",
> 1822 __func__, err);
> 1823 goto cleanup_netdev;
> 1824 }
> 1825
> 1826 devdata->rcvbuf = kcalloc(devdata->num_rcv_bufs,
> 1827 sizeof(struct sk_buff *), GFP_KERNEL);
> 1828 if (!devdata->rcvbuf) {
> 1829 err = -ENOMEM;
> 1830 goto cleanup_rcvbuf;
>
> We never allocated rcvbuf. It should still be goto cleanup_netdev; The
> other gotos after this in this function are slightly off as well. But
> thanks for the nice label names because it makes reviewing this much
> easier. :)
>
> 1831 }
>
> In visornic_init():
> 2126 err = visorbus_register_visor_driver(&visornic_driver);
> 2127 if (!err)
> 2128 return 0;
>
> Flip this around.
>
> if (err)
> goto cleanup_debugfs;
>
> return 0;
>
>
> These things should be reviewed by people on netdev and the scsi thing
> should get a review by the scsi people.
>
Appreciate the feedback Dan; we'll get right on cleaning this stuff up.
After we get these changes in, is there a specific process we need to
follow in order to have visornic inspected by the netdev folks
(assume netdev at vger.kernel.org) and visorhba inspected by the scsi folks
(assume linux-scsi at vger.kernel.org)?
I also assume we would need to do the same thing for visorinput
(linux-input at vger.kernel.org).
Thanks,
Tim Sell
> regards,
> dan carpenter
More information about the devel
mailing list