[firefly] staging: usbip: bugfix for isochronous packets and optimization

Dan Carpenter dan.carpenter at oracle.com
Tue Aug 26 21:47:09 EEST 2014


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 at oracle.com> 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.

> 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.

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.

regards,
dan carpenter



More information about the firefly mailing list