[haiku-bugs] Re: [Haiku] #7449: Check for invalid Frame List Sizes and enable Asynchronous Park Capability [gsoc2011]
- From: "gabriel.hartmann" <trac@xxxxxxxxxxxx>
- Date: Tue, 19 Apr 2011 22:42:05 -0000
#7449: Check for invalid Frame List Sizes and enable Asynchronous Park
Capability
[gsoc2011]
--------------------------------+----------------------------
Reporter: gabriel.hartmann | Owner: mmlr
Type: bug | Status: new
Priority: normal | Milestone: R1
Component: Drivers/USB | Version: R1/Development
Resolution: | Keywords: EHCI gsoc2011
Blocked By: | Blocking:
Has a Patch: 1 | Platform: All
--------------------------------+----------------------------
Comment (by gabriel.hartmann):
> * Since you don't use the "running" variable anymore, don't move it to
the top, keep it where it was, i.e. define it where it's used.
Fixed.
> * The output you do in case of encountering the unexpected frame list
size doesn't really make sense: "host controller didn't start: invalid
frame list size" seems to be a copy of the output that happens further
down with "invalid frame list size" added. The "host controller didn't
start" part should be dropped, as that's not what happened (that output is
correct only after trying to start it but failing, i.e. what happens
below). For the "invalid frame list size" part, I don't really like to
word things like this way in case of "reserved" stuff. It is not invalid,
it is reserved and I'd hate to have Haiku complain like that if it ever is
unreserved. More correct IMO in this case would be "unsupported frame list
size (%lu)", also outputting the offending value.
I've changed the error message to the one you recommend. I originally
retained the "host controller didn't start:" portion because there was a
trace statement at the top that said "starting EHCI host controller," and
because of the early return the error statement "host controller didn't
start." never got reached.
> * If you look at the patch, it is obvious that you added some stuff and
moved some parts around a few times. Ideally you'd clean up such leftovers
once the changes are complete. In this case, remove the extra blank lines
and the flip of the blank line and the trace as they don't belong with the
actual change (and don't really fit the coding style of the rest of the
file anyway). At the same time you'd notice the superfluous moving of the
"running" variable, as already mentioned. Basically the fewer extra
changes a patch includes the easier it is to understand what exactly was
done.
I've cleaned up the patch so that it's obvious what's been changed and
there's no superfluous stuff hanging around.
In regards to early return style, I'm happy to adopt it. It seems more
natural to me in any case. I was only avoiding it because I'd been taught
multiple return statements are bad.
--
Ticket URL: <http://dev.haiku-os.org/ticket/7449#comment:12>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
