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@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
