Re: Time for a code audit?
On Mon, May 02, 2016 at 04:38:27PM +, Sell, Timothy C wrote: > 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 net...@vger.kernel.org) and visorhba inspected by the scsi folks > (assume linux-s...@vger.kernel.org)? > > I also assume we would need to do the same thing for visorinput > (linux-in...@vger.kernel.org). Yeah. Just send it to those lists. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: Time for a code audit?
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Monday, May 02, 2016 8:55 AM
> To: Kershner, David A
> Cc: gre...@linuxfoundation.org; driverdev-devel@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,
>214unsigned int frags_max,
>215struct 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,
> 1827sizeof(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
> ea
Re: Time for a code audit?
Reviewing drivers/staging/unisys/visorinput/visorinput.c
465 case KEY_NUMLOCK:
466 led = LED_NUML;
467 break;
468 default:
469 led = -1;
Just make this a direct return.
470 break;
471 }
472 if (led >= 0) {
Then you can remove this if condition.
473 int old_state = (test_bit(led, visorinput_dev->led));
Remove extra parens.
474
475 if (old_state != desired_state) {
476 input_report_key(visorinput_dev, keycode, 1);
477 input_sync(visorinput_dev);
478 input_report_key(visorinput_dev, keycode, 0);
479 input_sync(visorinput_dev);
480 __change_bit(led, visorinput_dev->led);
481 }
482 }
483 }
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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,
214unsigned int frags_max,
215struct 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,
1827sizeof(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.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
Reviewing drivers/staging/unisys/visorhba/visorhba_main.c visor_thread_start() I'm nervous about the error handling here. Is setting ->id to zero really sufficient to prevent dereferencing ->task? Still a lot of return -1; grep "return -1;" drivers/staging/unisys/ -R del_scsipending_ent() flip the if condition around so it's error handling instead of success handling. Remove the "sent = NULL" initializer. drivers/staging/unisys/visorhba/visorhba_main.c 301 /* specify the event that has to be triggered when this */ 302 /* cmd is complete */ 303 cmdrsp->scsitaskmgmt.notify_handle = (u64)¬ifyevent; 304 cmdrsp->scsitaskmgmt.notifyresult_handle = (u64)¬ifyresult; This casting an int pointer to u64 pointer seems bad. I must be blind. How does queue_disk_add_remove() work? Where do we initialize dar_work_queue? process_disk_notify() switch success handling to error handling. visorhba_probe(). We really can only probe one device right? Because VISORHBA_OPEN_MAX is 1. So what happens if we try to probe two? I don't immediately see how that is handled. visorhba_init() return a literal zero on success. Looks pretty good generally to mee. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: Time for a code audit?
> -Original Message- > From: Romer, Benjamin M > Sent: Tuesday, February 16, 2016 10:03 AM > To: Dan Carpenter > Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; > Sell, Timothy C ; *S-Par-Maintainer > > Subject: Re: Time for a code audit? > ... > > Hi Dan, > > Thank you for this list! We'll get started on fixing these issues > immediately. I've run Smatch using the make options > > O=../builds/testbuild CC=gcc-4.9 CHECK="smatch -p=kernel" C=1 > > but when I use it this way, I don't get that line where it complains > about the variable's size. What settings do you use? > > If I can generate a list of problems that way across the whole driver > set, we can just resolve them all without having to bother you or Greg > until we have patches for them. > > Thanks a ton for the review. :) > > -- Ben Greg, With the patch "[PATCH] staging: unisys: visorbus: initialize variables", I believe all the issues brought up by both you and Dan are addressed by the patches you accepted. Also during the time we did some additional cleanup. What is the next step to get visorbus out of the staging directory? What do we do with the include files? Also, I'm working on removing the MODULE_ALIAS lines from visorinput, visornic, and visorhba. I believe this requires me to make changes in include/kernel/mod_devicemodules.h and scripts/mod/*. Should I send those fixes to the respective maintainers now or when we move the other drivers from staging? Thanks, David Kershner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: Time for a code audit?
> -Original Message- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Tuesday, February 23, 2016 3:26 PM > To: Kershner, David A > Cc: Romer, Benjamin M ; > gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; Sell, > Timothy C ; *S-Par-Maintainer > > Subject: Re: Time for a code audit? > > On Tue, Feb 23, 2016 at 05:45:55PM +, Kershner, David A wrote: > > > > > > > -Original Message- > > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > > Sent: Friday, February 12, 2016 5:02 PM > > > To: Romer, Benjamin M > > > Cc: gre...@linuxfoundation.org; driverdev- > de...@linuxdriverproject.org; > > > Sell, Timothy C ; *S-Par-Maintainer > > > > > > Subject: Re: Time for a code audit? > > > > > > I looked at the Smatch warnings, plus some bonus stuff I'm still working > > > on. > > > > > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > > > warn: inconsistent indenting > > > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > > > warn: inconsistent indenting > > > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > > > warn: XXX should 'inp_pfn + i' be a 64 bit type? > > > > > > This is because the left side is u64 but we wrap at u32. > > > > > > > Dan, thanks for the find. Though I'm curious what options are you using > when you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64 > bit type" message? When I run smatch locally I don't see that message but I > see the other two and it is definitely a problem in the codebase that I'm > looking at. > > > > These are things I'm still working on and haven't published yet. It > feels like it should be a warning though. I'm not certain the heuristic > to use here. Sometimes people declare things as u64 when they don't > care. Also Smatch tries to be clever about if the integer overflow is > actually possible but sometimes it still gets false positives. > > regards, > dan carpenter Is it possible for me to get a copy of your patches so I can verify that my fix doesn't break anything else? David ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
On Tue, Feb 23, 2016 at 05:45:55PM +, Kershner, David A wrote: > > > > -Original Message- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Friday, February 12, 2016 5:02 PM > > To: Romer, Benjamin M > > Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; > > Sell, Timothy C ; *S-Par-Maintainer > > > > Subject: Re: Time for a code audit? > > > > I looked at the Smatch warnings, plus some bonus stuff I'm still working > > on. > > > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > > warn: inconsistent indenting > > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > > warn: inconsistent indenting > > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > > warn: XXX should 'inp_pfn + i' be a 64 bit type? > > > > This is because the left side is u64 but we wrap at u32. > > > > Dan, thanks for the find. Though I'm curious what options are you using when > you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64 bit > type" message? When I run smatch locally I don't see that message but I see > the other two and it is definitely a problem in the codebase that I'm looking > at. > These are things I'm still working on and haven't published yet. It feels like it should be a warning though. I'm not certain the heuristic to use here. Sometimes people declare things as u64 when they don't care. Also Smatch tries to be clever about if the integer overflow is actually possible but sometimes it still gets false positives. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: Time for a code audit?
> -Original Message- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Friday, February 12, 2016 5:02 PM > To: Romer, Benjamin M > Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; > Sell, Timothy C ; *S-Par-Maintainer > > Subject: Re: Time for a code audit? > > I looked at the Smatch warnings, plus some bonus stuff I'm still working > on. > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > warn: XXX should 'inp_pfn + i' be a 64 bit type? > > This is because the left side is u64 but we wrap at u32. > Dan, thanks for the find. Though I'm curious what options are you using when you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64 bit type" message? When I run smatch locally I don't see that message but I see the other two and it is definitely a problem in the codebase that I'm looking at. David > drivers/staging/unisys/visorbus/visorbus_main.c:787 > visordriver_probe_device() warn: 'sem:&dev->visordriver_callback_lock' is > sometimes locked here and sometimes unlocked. > drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn: > XXX passing uninitialized 'tool_action' > drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX > passing uninitialized 'error' > drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX > passing uninitialized 'text_id' > drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show() > warn: XXX passing uninitialized 'remaining_steps' > > These with XXX are if channel->nbytes is too small. > > drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() warn: > user controlled 'adjustment' cast to postive rl = 's64min-s64max' > > This is because we read a s64 and pass it to a function as u64. > > Do an: grep "rc = -1" drivers/staging/unisys/ -R > > Also "if (rc != 0)" is a double negative. != zero is good style when > you are talking about the number zero or for strcmp() functions. > > Ho ho ho. > int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev- > >devnodes[0]); > > You guys know that when I read > drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot > that makes me itch. All the unnecessary initialization to -1 and the > goto aways. > > char s[99]; Why 99? Why s? I'm not a fan of a lot of the naming. > What is s? What is x? > > Blank lines after declaration sections. > >781 fix_vbus_dev_info(dev); >782 up(&dev->visordriver_callback_lock); >783 rc = 0; >784 away: >785 if (rc != 0) >786 put_device(&dev->device); >787 return rc; >788 } > > This is like in my kitchen because when I'm making spaghetti I throw > some against the wall to see if it is done and eventual the whole wall > has tiny spots of pasta stuck to it. > > I'm half way through the first file and this code is *almost* so close > to being ready, but could you just go through it once more and do a > final tidy up. > > regards, > dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
On Sat, 2016-02-13 at 01:01 +0300, Dan Carpenter wrote: > I looked at the Smatch warnings, plus some bonus stuff I'm still > working > on. > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > warn: XXX should 'inp_pfn + i' be a 64 bit type? > Hi Dan, Thank you for this list! We'll get started on fixing these issues immediately. I've run Smatch using the make options O=../builds/testbuild CC=gcc-4.9 CHECK="smatch -p=kernel" C=1 but when I use it this way, I don't get that line where it complains about the variable's size. What settings do you use? If I can generate a list of problems that way across the whole driver set, we can just resolve them all without having to bother you or Greg until we have patches for them. Thanks a ton for the review. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
On Mon, Feb 15, 2016 at 05:35:01PM +, Sell, Timothy C wrote:
> Re spaghetti:
>
> I certainly agree that most uses of 'goto' are evil.
> But I've found that when 'goto' is used to provide a single common
> exit-point for a function (e.g., "goto away"), it can lead to code
> that is much more readable and much less fragile than the alternative
> of sprinkling 'return's at various points within the function,
> especially when different clean-up actions are needed prior to each
> of those 'return's.
>
> For me, knowing that there is exactly one place where a function can
> return (i.e., at the end), and exactly one place where we perform
> resource clean-ups (i.e., at the 'away' label) makes it easier to write
> correct code, and I think easier to understand. 'away' essentially
> fills a destructor-like role.
>
> I don't consider such a use of 'goto' as 'spaghetti code'.
>
> I agree that in trivial functions, this strategy would be overkill,
> and I suspect this may indeed be what is at the core of your
> argument against it.
I've written a lot about error handling.
https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
Do everything labels are the most error prone way of doing error
handling. I discovered this because I look at a lot of error handling
code but it makes sense because doing everything is more complicated
than doing one thing.
I have also looked at does one err style error handling prevent future
bugs. For example, when people introduce a new return -ENOMEM; without
dropping the lock, does it matter if there is an out: label at the
bottom of the function and basically it does not. The people who don't
care about locks don't care about common exit paths either.
This style also causes future bugs when we change from a NULL on error
kmalloc(), copy_from_user() to an error pointer function like
kmemdup_user(). If you use canonical unwinding then you don't have that
problem because you don't free things which have not been allocated.
One err style causes bugs in the present and it causes bugs in the
future. And empirically, historically it doesn't prevented people
from adding new returns in the middle of functions.
> >781 fix_vbus_dev_info(dev);
> >782 up(&dev->visordriver_callback_lock);
> >783 rc = 0;
> >784 away:
> >785 if (rc != 0)
> >786 put_device(&dev->device);
> >787 return rc;
> >788 }
What makes this spaghetti code is that we twist the error paths and the
success paths together. I put it in my email because it was ugly, but
looking at it now I see it's also buggy. It's hard to argue this
prevents future bugs when it is buggy right now as-is.
Of course, that's a sample size of one. Let's look at the rest of
the file.
visorbus_match(), it's impossible to tell if the error codes are
intentional which is typical for do-nothing gotos. The comment is not
totally helpful.
343 away:
344 if (rc < 0) {
345 kfree(myattr);
346 myattr = NULL;
347 dev->devnodes[slot].attr = NULL;
348 }
349 return rc;
350 }
Memory corruption. Gar, Smatch should have caught this.
create_visor_device() has incomplete error handling.
1460 rc = 0;
1461 away:
1462 if (rc < 0) {
1463 if (notify_func)
1464 (*notify_func)(dev, rc);
1465 }
1466 }
Hm... This one is tricky. rc is -1. notify_func is either
visorchipset_device_pause_response() or device_resume_response(). I'm
a bit confused about ->pending_msg_hdr and how that is protected from
races. We do "msg->hdr.completion_status = (u32)(-response);" where
response is -1. It's hard to tell what's going on here. I think
normally we pass positive error codes to response like
CONTROLVM_RESP_ERROR_DEVICE_UDEV_TIMEOUT (1400)?
That's the end of the file. So most of the time if the away label does
something more complicated than printing a message and and returning
it's probably buggy.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: Time for a code audit?
Re spaghetti: I certainly agree that most uses of 'goto' are evil. But I've found that when 'goto' is used to provide a single common exit-point for a function (e.g., "goto away"), it can lead to code that is much more readable and much less fragile than the alternative of sprinkling 'return's at various points within the function, especially when different clean-up actions are needed prior to each of those 'return's. For me, knowing that there is exactly one place where a function can return (i.e., at the end), and exactly one place where we perform resource clean-ups (i.e., at the 'away' label) makes it easier to write correct code, and I think easier to understand. 'away' essentially fills a destructor-like role. I don't consider such a use of 'goto' as 'spaghetti code'. I agree that in trivial functions, this strategy would be overkill, and I suspect this may indeed be what is at the core of your argument against it. > -Original Message- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Friday, February 12, 2016 5:02 PM > To: Romer, Benjamin M > Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; > Sell, Timothy C; *S-Par-Maintainer > Subject: Re: Time for a code audit? > > I looked at the Smatch warnings, plus some bonus stuff I'm still working > on. > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > warn: XXX should 'inp_pfn + i' be a 64 bit type? > > This is because the left side is u64 but we wrap at u32. > > drivers/staging/unisys/visorbus/visorbus_main.c:787 > visordriver_probe_device() warn: 'sem:&dev->visordriver_callback_lock' is > sometimes locked here and sometimes unlocked. > drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn: > XXX passing uninitialized 'tool_action' > drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX > passing uninitialized 'error' > drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX > passing uninitialized 'text_id' > drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show() > warn: XXX passing uninitialized 'remaining_steps' > > These with XXX are if channel->nbytes is too small. > > drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() > warn: user controlled 'adjustment' cast to postive rl = 's64min-s64max' > > This is because we read a s64 and pass it to a function as u64. > > Do an: grep "rc = -1" drivers/staging/unisys/ -R > > Also "if (rc != 0)" is a double negative. != zero is good style when > you are talking about the number zero or for strcmp() functions. > > Ho ho ho. > int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev- > >devnodes[0]); > > You guys know that when I read > drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot > that makes me itch. All the unnecessary initialization to -1 and the > goto aways. > > char s[99]; Why 99? Why s? I'm not a fan of a lot of the naming. > What is s? What is x? > > Blank lines after declaration sections. > >781 fix_vbus_dev_info(dev); >782 up(&dev->visordriver_callback_lock); >783 rc = 0; >784 away: >785 if (rc != 0) >786 put_device(&dev->device); >787 return rc; >788 } > > This is like in my kitchen because when I'm making spaghetti I throw > some against the wall to see if it is done and eventual the whole wall > has tiny spots of pasta stuck to it. > > I'm half way through the first file and this code is *almost* so close > to being ready, but could you just go through it once more and do a > final tidy up. > > regards, > dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
On Fri, Feb 12, 2016 at 02:01:06PM -0500, Ben Romer wrote: > Greg, > > Thank you very much for taking our patches. All of us here appreciate > it a lot. :) > > It looks to me like our driver set is clean as far as > checkpatch.pl is concerned - there's one error, which was said to be > acceptable as it was, and a small number of checks that can't be > made any prettier. > > I was hoping we could get started with the code audit and work on > getting the drivers out of the staging tree. What's the best way to > do that? For some reason I thought the "raw kobject" usage was going to be converted to use 'struct device' so that you can then take advantage of userspace tools to read/write the sysfs attributes properly (i.e. libudev). As long as they are kobjects, userspace is "blind" to them. I know I've brought this up before, but I keep forgetting why the code is this way. If it can only be done with kobjects, then we need some huge comments in the code as to why this is so. Also, mostcore.h has a function that returns a kobject, that seems "odd" to me, shouldn't it be returning a object type of your bus/subsystem instead? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
I looked at the Smatch warnings, plus some bonus stuff I'm still working on. drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() warn: inconsistent indenting drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() warn: inconsistent indenting drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() warn: XXX should 'inp_pfn + i' be a 64 bit type? This is because the left side is u64 but we wrap at u32. drivers/staging/unisys/visorbus/visorbus_main.c:787 visordriver_probe_device() warn: 'sem:&dev->visordriver_callback_lock' is sometimes locked here and sometimes unlocked. drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn: XXX passing uninitialized 'tool_action' drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX passing uninitialized 'error' drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX passing uninitialized 'text_id' drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show() warn: XXX passing uninitialized 'remaining_steps' These with XXX are if channel->nbytes is too small. drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() warn: user controlled 'adjustment' cast to postive rl = 's64min-s64max' This is because we read a s64 and pass it to a function as u64. Do an: grep "rc = -1" drivers/staging/unisys/ -R Also "if (rc != 0)" is a double negative. != zero is good style when you are talking about the number zero or for strcmp() functions. Ho ho ho. int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev->devnodes[0]); You guys know that when I read drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot that makes me itch. All the unnecessary initialization to -1 and the goto aways. char s[99]; Why 99? Why s? I'm not a fan of a lot of the naming. What is s? What is x? Blank lines after declaration sections. 781 fix_vbus_dev_info(dev); 782 up(&dev->visordriver_callback_lock); 783 rc = 0; 784 away: 785 if (rc != 0) 786 put_device(&dev->device); 787 return rc; 788 } This is like in my kitchen because when I'm making spaghetti I throw some against the wall to see if it is done and eventual the whole wall has tiny spots of pasta stuck to it. I'm half way through the first file and this code is *almost* so close to being ready, but could you just go through it once more and do a final tidy up. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Time for a code audit?
Greg, Thank you very much for taking our patches. All of us here appreciate it a lot. :) It looks to me like our driver set is clean as far as checkpatch.pl is concerned - there's one error, which was said to be acceptable as it was, and a small number of checks that can't be made any prettier. I was hoping we could get started with the code audit and work on getting the drivers out of the staging tree. What's the best way to do that? Again, thanks for taking the time to put our cleanups in. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
