From: Alan Stern <stern@rowland.harvard.edu>
To: Tom Todd <thomas.m.a.todd@gmail.com>
Cc: gregkh@linuxfoundation.org, <joe@perches.com>,
<dan.carpenter@oracle.com>, <arvind.yadav.cs@gmail.com>,
<linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] USB: core: devio: Restructured proc_ioctl for readability
Date: Sun, 12 Aug 2018 18:46:34 -0400 (EDT) [thread overview]
Message-ID: <Pine.LNX.4.44L0.1808121838570.23021-100000@netrider.rowland.org> (raw)
In-Reply-To: <20180812222653.3341-1-thomas.m.a.todd@gmail.com>
On Sun, 12 Aug 2018, Tom Todd wrote:
> On, Sun, 12 Aug 2018 11:34:59 -0700 Joe Perches wrote:
> >On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
> >> Fixed a code styling issue
> >while it's OK to fix style only issues, it's much better
> >to reorganize the code for reader clarity.
>
> Ok, thank you, I've taken your suggestions and created the a new version
>
> Restructured method proc_ioctl for readability and fixed code style
> errors.
>
> Signed-off-by: Tom Todd <thomas.m.a.todd@gmail.com>
"Style fixes" that introduce bugs are quite annoying...
> ---
> drivers/usb/core/devio.c | 67 ++++++++++++++++++++++------------------
> 1 file changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 476dcc5f2da3..740e60e086e2 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -2117,46 +2117,52 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> /* alloc buffer */
> size = _IOC_SIZE(ctl->ioctl_code);
> if (size > 0) {
> - buf = kmalloc(size, GFP_KERNEL);
> - if (buf == NULL)
> - return -ENOMEM;
> - if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
> - if (copy_from_user(buf, ctl->data, size)) {
> - kfree(buf);
> + if (_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE) {
> + buf = memdup_user(ctl->data, size);
> + if (!buf)
> return -EFAULT;
> - }
> } else {
> - memset(buf, 0, size);
> + buf = kmalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
Where do the contents of buf get set to 0 now?
> }
> }
>
> if (!connected(ps)) {
> - kfree(buf);
> - return -ENODEV;
> + retval = -ENODEV;
> + goto exit;
> }
>
> - if (ps->dev->state != USB_STATE_CONFIGURED)
> + if (ps->dev->state != USB_STATE_CONFIGURED) {
> retval = -EHOSTUNREACH;
> - else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
> + goto exit;
> + }
> +
> + intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
> + if (!intf) {
> retval = -EINVAL;
> - else switch (ctl->ioctl_code) {
> + goto exit;
> + }
>
> + switch (ctl->ioctl_code) {
> /* disconnect kernel driver from interface */
> case USBDEVFS_DISCONNECT:
> - if (intf->dev.driver) {
> - driver = to_usb_driver(intf->dev.driver);
> - dev_dbg(&intf->dev, "disconnect by usbfs\n");
> - usb_driver_release_interface(driver, intf);
> - } else
> + if (!intf->dev.driver) {
> retval = -ENODATA;
> + goto exit;
> + }
> + driver = to_usb_driver(intf->dev.driver);
> + dev_dbg(&intf->dev, "disconnect by usbfs\n");
> + usb_driver_release_interface(driver, intf);
> break;
>
> /* let kernel drivers try to (re)bind to the interface */
> case USBDEVFS_CONNECT:
> - if (!intf->dev.driver)
> - retval = device_attach(&intf->dev);
> - else
> + if (!intf->dev.driver) {
Now !intf->dev.driver generates an error instead of working?
> retval = -EBUSY;
> + goto exit;
> + }
> + retval = device_attach(&intf->dev);
> break;
>
> /* talk directly to the interface's driver */
> @@ -2165,20 +2171,21 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> driver = to_usb_driver(intf->dev.driver);
> if (driver == NULL || driver->unlocked_ioctl == NULL) {
> retval = -ENOTTY;
> - } else {
> - retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
> - if (retval == -ENOIOCTLCMD)
> - retval = -ENOTTY;
> + goto exit;
> }
> + retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
> + if (retval == -ENOIOCTLCMD) {
> + retval = -ENOTTY;
> + goto exit;
> + }
> + break;
> }
>
> /* cleanup and return */
> - if (retval >= 0
> - && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0
> - && size > 0
> - && copy_to_user(ctl->data, buf, size) != 0)
> + if (retval >= 0 && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0 &&
> + size > 0 && copy_to_user(ctl->data, buf, size) != 0)
> retval = -EFAULT;
Isn't the style in this file to indent continuation lines by two tab
stops?
> -
> +exit:
> kfree(buf);
> return retval;
> }
Alan Stern
next prev parent reply other threads:[~2018-08-12 22:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-12 18:06 [PATCH] USB: core: devio: fixed a trailing statement code style issue Tom Todd
2018-08-12 18:34 ` Joe Perches
2018-08-12 22:26 ` [PATCH v2] USB: core: devio: Restructured proc_ioctl for readability Tom Todd
2018-08-12 22:46 ` Alan Stern [this message]
2018-08-13 6:34 ` Greg KH
2018-08-13 18:22 ` thomas.m.a.todd
2018-08-12 21:50 ` [PATCH] USB: core: devio: fixed a trailing statement code style issue Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.44L0.1808121838570.23021-100000@netrider.rowland.org \
--to=stern@rowland.harvard.edu \
--cc=arvind.yadav.cs@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=thomas.m.a.todd@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).