lkml.org 
[lkml]   [2013]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv3 02/16] staging: usbip: Add kernel support for client ACLs
    The main thing is that there is an IS_ERR vs NULL check.  The rest is
    just style nits.

    On Sat, Sep 28, 2013 at 07:42:39PM +0200, Dominik Paulus wrote:
    > +static ssize_t store_acl(struct device *dev, struct device_attribute *attr,
    > + const char *buf, size_t count)
    > +{
    > + struct stub_device *sdev = dev_get_drvdata(dev);
    > + int retval = 0;
    > +
    > + if (!sdev)
    > + return -ENODEV;

    Can this even happen? If not, don't check for impossible conditions it
    is sloppy.

    > +
    > + if (count >= PAGE_SIZE)
    > + /* Prevent storing oversized ACLs in kernel memory */
    > + return -EINVAL;
    > +

    Put parenthesis around multi-line indents. But in this case just delete
    the obvious comment.

    > + /* Store ACL */

    Delete obvious comment.

    > + spin_lock_irq(&sdev->ip_lock);
    > + kfree(sdev->acls);
    > + sdev->acls = kstrdup(buf, GFP_KERNEL);

    kstrdup() returns a NULL on error not an error pointer. Configure vim
    to use cscope and run make cscope.

    > + if (IS_ERR(sdev->acls)) {
    > + retval = PTR_ERR(sdev->acls);
    > + sdev->acls = NULL;
    > + } else {
    > + retval = strlen(sdev->acls);
    > + }
    > + spin_unlock_irq(&sdev->ip_lock);
    > +
    > + return retval;
    > +}
    > +
    > +/*
    > + * This functions prints all allowed IP addrs for this dev
    > + */
    > +static ssize_t show_acl(struct device *dev, struct device_attribute *attr,
    > + char *buf)
    > +{
    > + struct stub_device *sdev = dev_get_drvdata(dev);
    > + int retval = 0;

    Don't initialize things if you don't use the value. (It stops GCC from
    warning about uninitialized variables properly).

    > +
    > + if (!sdev)
    > + return -ENODEV;
    > +
    > + spin_lock_irq(&sdev->ip_lock);
    > + if (sdev->acls == NULL) {
    > + retval = 0;
    > + } else {
    > + strcpy(buf, sdev->acls);
    > + retval = strlen(buf);
    > + }
    > + spin_unlock_irq(&sdev->ip_lock);
    > +
    > + return retval;
    > +}
    > +static DEVICE_ATTR(usbip_acl, S_IWUSR | S_IRUGO, show_acl, store_acl);
    > +
    > static int stub_add_files(struct device *dev)
    > {
    > int err = 0;
    > @@ -157,9 +213,13 @@ static int stub_add_files(struct device *dev)
    > err = device_create_file(dev, &dev_attr_usbip_debug);
    > if (err)
    > goto err_debug;
    > + err = device_create_file(dev, &dev_attr_usbip_acl);
    > + if (err)
    > + goto err_ip;
    >
    > return 0;
    > -
    > +err_ip:
    > + device_remove_file(dev, &dev_attr_usbip_debug);
    > err_debug:
    > device_remove_file(dev, &dev_attr_usbip_sockfd);
    > err_sockfd:

    I wasn't able to figure out what the "ip" part of "err_ip" meant...

    Really these are all poor label names. They are based on the goto
    location instead of the label location.

    > @@ -173,6 +233,7 @@ static void stub_remove_files(struct device *dev)
    > device_remove_file(dev, &dev_attr_usbip_status);
    > device_remove_file(dev, &dev_attr_usbip_sockfd);
    > device_remove_file(dev, &dev_attr_usbip_debug);
    > + device_remove_file(dev, &dev_attr_usbip_acl);
    > }
    >
    > static void stub_shutdown_connection(struct usbip_device *ud)
    > @@ -306,12 +367,14 @@ static struct stub_device *stub_device_alloc(struct usb_device *udev,
    > sdev->ud.status = SDEV_ST_AVAILABLE;
    > spin_lock_init(&sdev->ud.lock);
    > sdev->ud.tcp_socket = NULL;
    > + sdev->acls = NULL;

    This isn't really needed because we allocated sdev with kzalloc().


    >
    > INIT_LIST_HEAD(&sdev->priv_init);
    > INIT_LIST_HEAD(&sdev->priv_tx);
    > INIT_LIST_HEAD(&sdev->priv_free);
    > INIT_LIST_HEAD(&sdev->unlink_free);
    > INIT_LIST_HEAD(&sdev->unlink_tx);
    > + spin_lock_init(&sdev->ip_lock);
    > spin_lock_init(&sdev->priv_lock);
    >
    > init_waitqueue_head(&sdev->tx_waitq);
    > @@ -511,6 +574,9 @@ static void stub_disconnect(struct usb_interface *interface)
    > usb_put_dev(sdev->udev);
    > usb_put_intf(interface);
    >
    > + /* free ACL list */
    > + kfree(sdev->acls);
    > +
    > /* free sdev */
    > busid_priv->sdev = NULL;
    > stub_device_free(sdev);
    > --
    > 1.8.4
    >
    > _______________________________________________
    > devel mailing list
    > devel@linuxdriverproject.org
    > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


    \
     
     \ /
      Last update: 2013-09-30 10:41    [W:2.346 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site