From: Joe Perches <joe@perches.com>
To: Tom Todd <thomas.m.a.todd@gmail.com>, gregkh@linuxfoundation.org
Cc: stern@rowland.harvard.edu, dan.carpenter@oracle.com,
arvind.yadav.cs@gmail.com, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] USB: core: devio: fixed a trailing statement code style issue
Date: Sun, 12 Aug 2018 11:34:59 -0700 [thread overview]
Message-ID: <10f9f13a5421299a6efda1e1f84ab304ef18ddec.camel@perches.com> (raw)
In-Reply-To: <20180812180656.26079-1-thomas.m.a.todd@gmail.com>
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.
For this code, something like:
o use memdup_user
o use an exit label to free allocated memory
o invert tests to return on unexpected results
o move logical continuations to EOL
o don't use successive code like:
if (foo)
return -ERRNO;
else if (bar)
return -ERRNO;
else
baz;
simpler and less indentation is:
if (foo)
return -ERRNO;
if (bar)
return -ERRNO;
baz;
---
drivers/usb/core/devio.c | 68 ++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6ce77b33da61..74ae4e052357 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2118,68 +2118,74 @@ 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 = kzalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
}
}
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) {
retval = -EBUSY;
+ goto exit;
+ }
+ retval = device_attach(&intf->dev);
break;
/* talk directly to the interface's driver */
default:
if (intf->dev.driver)
driver = to_usb_driver(intf->dev.driver);
- if (driver == NULL || driver->unlocked_ioctl == NULL) {
+ if (!driver || !driver->unlocked_ioctl) {
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;
+exit:
kfree(buf);
return retval;
}
next prev parent reply other threads:[~2018-08-12 18:35 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 [this message]
2018-08-12 22:26 ` [PATCH v2] USB: core: devio: Restructured proc_ioctl for readability Tom Todd
2018-08-12 22:46 ` Alan Stern
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=10f9f13a5421299a6efda1e1f84ab304ef18ddec.camel@perches.com \
--to=joe@perches.com \
--cc=arvind.yadav.cs@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=stern@rowland.harvard.edu \
--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).