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