On Fri 21 Oct 2016 06:21:59 PM CEST, Pradeep Jagadeesh
<pradeepkiruv...@gmail.com> wrote:
> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write)
> +{
> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
^
There's an extra whitespace there.
> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool
> is_write)
> +{
> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
> + if (fsdev_throttle_check_for_wait(fst, is_write)) {
> + return;
> + }
> + }
> + qemu_co_queue_next(&fst->throttled_reqs[is_write]);
> +}
That is wrong, you're calling qemu_co_queue_next() even if the queue is
empty. That line should be inside the 'if' block.
I still think that -after the changes you made in v6- you could put the
contents of that whole function inside fsdev_co_throttle_request(), and
at the same time get rid of fsdev_throttle_check_for_wait() and call
throttle_schedule_timer() directly. The resulting code will be smaller
and probably easier to read. But those are just style decisions.
> +static int get_num_bytes(struct iovec *iov, int iovcnt)
> +{
> + int i;
> + int bytes = 0;
> +
> + for (i = 0; i < iovcnt; i++) {
> + bytes += iov[i].iov_len;
> + }
> + return bytes;
> +}
I overlooked that in my previous review, but 'bytes' should be unsigned
(both here and in fsdev_co_throttle_request).
I still have to review the init/cleanup functions (I'll try to do it on
monday) but else the code looks fine now.
Berto
