On Tue, Aug 26, 2014 at 11:47 AM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> wrote:
> On Tue, Aug 26, 2014 at 09:06:17AM -0700, Valentina Manea wrote:
>> On Tue, Aug 26, 2014 at 6:12 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>> > 707 /*
>> > 708 * loop over all packets from last to first (to prevent overwritting
>> > 709 * memory when padding) and move them into the proper place
>> > 710 */
>> > 711 for (i = np-1; i > 0; i--) {
>> >
>> > I don't understand this loop. If we really intended to skip the first
>> > packet then why isn't the "if (np == 0" condition "if (np <= 1"? That
>> > would be more clear and the comment would say "if 1 packet then nothing
>> > to unpack."
>> >
>> > 712 actualoffset -= urb->iso_frame_desc[i].actual_length;
>> > 713 memmove(urb->transfer_buffer + urb->iso_frame_desc[i].offset,
>> > 714 urb->transfer_buffer + actualoffset,
>> > 715 urb->iso_frame_desc[i].actual_length);
>> > 716 }
>> > 717 }
>> >
>>
>> urb->actual_length is the sum of individual packets' lengths,
>> urb->iso_frame_desc[i].actual_length. Since actualoffset in the loop
>> keeps decreasing, it will reach 0 when the first packet is processed.
>> The first packet's offset is 0 as well and this just does memmove to
>> the same buffer location.
>
> It actually won't do the memove() if the i = 0, because the for loop
> condition says i > 0.
>
Well, yes. What I said would happen if it wasn't i > 0. Having this
prevents a useless memmove.
>> The (np == 0) condition is *not* the same as (np <= 1) because we want
>> to return only is there are any packets.
>
> Read the code again. For np == 1 then it doesn't do anything because it
> doesn't enter the for loop. In other words, it returns without doing
> anything either way.
>
It just happens that for np == 1 doesn't do anything and returns but,
conceptually, having one packet
that doesn't need extra processing (for loop) is not the same as not
having any packets at all (np == 0)
and I believe we shouldn't mix up these conditions.
> I'm not necessarily saying the code is wrong... I just think that it
> would be more clear if we said explicitly that we should return without
> doing anything if np == 1.
>
I think just adding a comment about the for loop stating that the
first packet doesn't need any extra processing would suffice.
Thanks,
Valentina
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
