On 2012.08.01 12:52, Hans de Goede wrote:
> Hmm, I hadn't noticed the explicit disarm there, since the
> handle_timeouts_locked code just cancels
> transfers and does not do anything io-intensive, the run-time for
> handle_timeouts_locked will be
> short, so I see no value in the disarm there, and vote for removing it.
OK, then we have two votes here, which is good enough for me. I'll
remove the disarm call.
> > diff --git a/libusb/io.c b/libusb/io.c
> > index b8d445c..e034a6e 100644
> > --- a/libusb/io.c
> > +++ b/libusb/io.c
> > @@ -1159,7 +1159,7 @@ static int add_to_flying_list(struct
> usbi_transfer *transfer)
> > struct timeval *timeout = &transfer->timeout;
> > struct libusb_context *ctx = ITRANSFER_CTX(transfer);
> > int r = 0;
> > - int first = 1;
> > + int first = 0;
> >
> > usbi_mutex_lock(&ctx->flying_transfers_lock);
> >
> > @@ -1167,7 +1167,7 @@ static int add_to_flying_list(struct
> usbi_transfer *transfer)
> > if (list_empty(&ctx->flying_transfers)) {
> > list_add(&transfer->list, &ctx->flying_transfers);
> > if (timerisset(timeout))
> > - r = 1;
> > + first = 1;
> > goto out;
> > }
> >
> > @@ -1186,15 +1186,32 @@ static int add_to_flying_list(struct
> usbi_transfer *transfer)
> > (cur_tv->tv_sec == timeout->tv_sec &&
> > cur_tv->tv_usec > timeout->tv_usec)) {
> > list_add_tail(&transfer->list, &cur->list);
> > - r = first;
> > + first = 1;
> > goto out;
> > }
> > - first = 0;
> > }
> >
> > /* otherwise we need to be inserted at the end */
> > list_add_tail(&transfer->list, &ctx->flying_transfers);
> > out:
> > +#ifdef USBI_TIMERFD_AVAILABLE
>
> The changes to the usage of first / r above introduce a bug, if the
> transfer being
> added to the list is not the first one added, first will now still get
> set to 1,
> causing us to re-arm the timerfd to a timeout of a transfer which
> times-out later,
> which we of course should not do.
>
> I suggest just keeping the above code the same and changing:
> > + if (first && usbi_using_timerfd(ctx)) {
>
> To:
>
> > + if (r == 1 && usbi_using_timerfd(ctx)) {
Good point on the introduced bug, but I think I'd still prefer to keep
using a 'first' variable, even if it results in slightly more ugly code,
as long as it helps make the intent more explicit.
This is even more true as the original code is confusing enough (at
least as far as I'm concerned) to make me think we have yet another bug
on our hands here.
Don't you think the following:
> (cur_tv->tv_sec == timeout->tv_sec &&
> cur_tv->tv_usec > timeout->tv_usec)) {
> list_add_tail(&transfer->list, &cur->list);
Should actually be:
> (cur_tv->tv_sec == timeout->tv_sec &&
> cur_tv->tv_usec > timeout->tv_usec)) {
> list_add(&transfer->list, &cur->list);
From the comparison, our parameter (transfer)'s timeout is earlier than
the one from the current item (cur) we process, so using list_add_tail
seems highly suspicious. For instance, if the new transfer we add has
the lowest timeout of all, I can't see how using list_add_tail will
result in that transfer ever being placed first on our list...
> > r = usbi_backend->submit_transfer(itransfer);
> > if (r) {
> > usbi_mutex_lock(&ctx->flying_transfers_lock);
> > list_del(&itransfer->list);
> > usbi_mutex_unlock(&ctx->flying_transfers_lock);
> > }
>
> We should call arm_timerfd_for_next_timeout() after the list_del here to
> adjust the timer for the changes to the list.
Makes sense.
> > + r = arm_timerfd_for_next_timeout(ctx);
> > + if (r < 0) {
> > + usbi_mutex_unlock(&ctx->flying_transfers_lock);
> > return r;
> > + }
> > }
> > + usbi_mutex_unlock(&ctx->flying_transfers_lock);\
>
> Maybe move the if (r < 0) return r; block outside of the locked code block,
> and avoid having the unlock twice?
I think it was originally like that, and I thought it was more explicit
this way when considering code sections in the absolute (we have an
error, so we want to exit, but we need to release the lock first). If
you or somebody else think it's preferable the other way round, I don't
mind going with that.
Regards,
/Pete
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
