Gang:
I apologize for taking so long to respond to your patch. I didn't get
much work done during the holidays...
Overall it looks pretty good. There are a few things that I would
prefer to change.
On Thu, 20 Dec 2012, Chen Gang wrote:
> reason (why):
> for function uhci_sprint_schedule:
> the buffer len is MAX_OUTPUT: 64 * 1024
> the buffer may not be enough:
> may loop UHCI_NUMFRAMES times (UHCI_NUMFRAMES is 1024)
> each time of loop may get more than 64 bytes
> so need check the buffer length to avoid memory overflow
>
> goal (what):
> this patch fixes this correctness issue.
>
> design (and why):
> at first, we make enough room for buffering the exceeding contents
> judge the contents which we have written whether bigger than buffer length
> if bigger (the exceeding contents will be in the exceeding buffer)
> break current work flow, and return
>
> test:
> plan:
> let MAX_OUTPUT as various values:
> some values which are enough for use, then can get full contents.
> some values which small enough, so can truncate in various locations.
> check the result:
> cat the contents from the /sys/kernel/debug/usb/uhci/*
> use wc -c to get the count of output contents (match the MAX_OUTPUT)
>
> result:
> make the buffer size in different size: (1024 is the room for exceeding)
> 63 * 1024 + 1024
> 1st (debug == 3) pass
> 2nd (debug == 3) pass
> 3rd (debug == 1) pass
> 4rd (not define DEBUG) pass
> 1689 + 1024 (debug == 3) pass (1689 is full contents length of my test)
> 1688 + 1024 (debug == 3) pass
> 1024 + 1024 (debug == 3) pass
> 512 + 1024 (debug == 3) pass
> 30 + 1024 (debug == 3) pass
>
> left:
> not test it by calling from uhci-q.c and uhci-hcd.c
> not test the dump objects which already have rich data contents
> if testing them are necessary:
> please tell me, I will try.
> and it is better to tell me how to test them.
Can you rewrite this using ordinary English sentences? Describe what
you changed and why you changed it. You don't have to include the
details of your tests.
> --- a/drivers/usb/host/uhci-debug.c
> +++ b/drivers/usb/host/uhci-debug.c
> @@ -16,6 +16,8 @@
>
> #include "uhci-hcd.h"
>
> +#define LEFT_OUTPUT (1024)
A better name would be EXTRA_SPACE.
> @@ -90,7 +90,11 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct
> uhci_td *td, char *buf,
> spid);
> out += sprintf(out, "(buf=%08x)\n", hc32_to_cpu(uhci, td->buffer));
>
> +tail:
> return out - buf;
> +truncate:
> + out += sprintf(out, " (%s truncated)\n", __FUNCTION__);
> + goto tail;
> }
Here and in all the other functions, it would be better to do this:
+ done:
+ if (out - buf > len)
+ out += sprintf(out, " ...\n");
return out - buf;
}
You don't need to write the word "truncated" all the time. A simple
"..." will let people know what happened.
Have all the "goto" statements jump to "done". The reason for doing it
this way is...
> @@ -135,6 +142,8 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct
> urb_priv *urbp,
> out += sprintf(out, "%*s%d: ", space + 2, "", i);
> out += uhci_show_td(uhci, td, out,
> len - (out - buf), 0);
> + if (out - buf > len)
> + goto truncate;
... here you can jump directly to the return statement and avoid
printing out a duplicate "..." line.
> @@ -211,9 +224,12 @@ static int uhci_show_qh(struct uhci_hcd *uhci,
> space, "");
> i = nurbs = 0;
> list_for_each_entry(urbp, &qh->queue, node) {
> - if (++i <= 10)
> + if (++i <= 10) {
> out += uhci_show_urbp(uhci, urbp, out,
> len - (out - buf), space + 2);
> + if (out - buf > len)
> + goto truncate;
Same here.
> @@ -222,24 +238,25 @@ static int uhci_show_qh(struct uhci_hcd *uhci,
> space, "", nurbs);
> }
>
> + if (out - buf > len)
> + goto truncate;
> +
> if (qh->dummy_td) {
> out += sprintf(out, "%*s Dummy TD\n", space, "");
> out += uhci_show_td(uhci, qh->dummy_td, out,
> len - (out - buf), 0);
And here.
> @@ -360,9 +383,13 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci,
> char *buf, int len)
> "int8", "int4", "int2", "async", "term"
> };
>
> - out += uhci_show_root_hub_state(uhci, out, len - (out - buf));
> + out += uhci_show_root_hub_state(uhci, out);
> + if (out - buf > len)
> + goto truncate;
And here.
> out += sprintf(out, "HC status\n");
> out += uhci_show_status(uhci, out, len - (out - buf));
> + if (out - buf > len)
> + goto truncate;
And here... You get the idea.
> @@ -375,14 +402,19 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci,
> char *buf, int len)
> uhci_to_hcd(uhci)->self.bandwidth_int_reqs,
> uhci_to_hcd(uhci)->self.bandwidth_isoc_reqs);
> if (debug <= 1)
> - return out - buf;
> + goto tail;
>
> out += sprintf(out, "Frame List\n");
> + if (out - buf > len)
> + goto truncate;
This test isn't needed because there's another test inside the loop
below.
Other than these small points, I don't see any problems.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
