[Spice-devel] [PATCH 02/11] worker: simplify process loops

[Jonathon Jongsma]

Nice thorough explanation

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


On Tue, 2016-02-09 at 10:27 +0000, Frediano Ziglio wrote:
> It was not clear when req_cmd_notification was called.
> This code reproduce just the old behavior but is easier to read.
> 
> Steps are:
> 
> // original
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
>     return n;
> }
> if (worker->display_poll_tries > CMD_RING_POLL_RETRIES ||
>              worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>     worker->display_poll_tries++;
>     return n;
> }
> continue;
> 
> // split if
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
>     return n;
> }
> if (worker->display_poll_tries > CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>     worker->display_poll_tries++;
>     return n;
> }
> continue;
> 
> // order inside if
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->display_poll_tries > CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>     worker->display_poll_tries++;
>     return n;
> }
> continue;
> 
> // consider cases
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->display_poll_tries > CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->display_poll_tries == CMD_RING_POLL_RETRIES) {
>     if (worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>         worker->display_poll_tries++;
>         return n;
>     }
>     continue;
> }
> // unreachable
> 
> // invert if
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->display_poll_tries > CMD_RING_POLL_RETRIES) {
>     worker->display_poll_tries++;
>     return n;
> }
> if (worker->display_poll_tries == CMD_RING_POLL_RETRIES) {
>     if (!worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>         continue;
>     }
>     worker->display_poll_tries++;
>     return n;
> }
> // unreachable
> 
> // reuse code
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
> } else if (worker->display_poll_tries > CMD_RING_POLL_RETRIES) {
> } else if (worker->display_poll_tries == CMD_RING_POLL_RETRIES) {
>     if (!worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>         continue;
>     }
> }
> worker->display_poll_tries++;
> return n;
> 
> // remove empty if
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
> } else if (worker->display_poll_tries == CMD_RING_POLL_RETRIES) {
>     if (!worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>         continue;
>     }
> }
> worker->display_poll_tries++;
> return n;
> 
> // collapse two conditions
> 
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
>     worker->event_timeout = MIN(worker->event_timeout, CMD_RING_POLL_TIMEOUT);
> } else if (worker->display_poll_tries == CMD_RING_POLL_RETRIES &&
>            !worker->qxl->st->qif->req_cmd_notification(worker->qxl)) {
>     continue;
> }
> worker->display_poll_tries++;
> return n;
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-worker.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index e70c9df..ba37b6c 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -167,16 +167,13 @@ static int red_process_cursor(RedWorker *worker, int
> *ring_is_empty)
>          if (!worker->qxl->st->qif->get_cursor_command(worker->qxl, &ext_cmd))
> {
>              *ring_is_empty = TRUE;
>              if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
> -                worker->cursor_poll_tries++;
>                  worker->event_timeout = MIN(worker->event_timeout,
> CMD_RING_POLL_TIMEOUT);
> -                return n;
> +            } else if (worker->cursor_poll_tries == CMD_RING_POLL_RETRIES &&
> +                       !worker->qxl->st->qif->req_cursor_notification(worker
> ->qxl)) {
> +                continue;
>              }
> -            if (worker->cursor_poll_tries > CMD_RING_POLL_RETRIES ||
> -                worker->qxl->st->qif->req_cursor_notification(worker->qxl)) {
> -                worker->cursor_poll_tries++;
> -                return n;
> -            }
> -            continue;
> +            worker->cursor_poll_tries++;
> +            return n;
>          }
>          worker->cursor_poll_tries = 0;
>          switch (ext_cmd.cmd.type) {
> @@ -228,16 +225,13 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
>          if (!worker->qxl->st->qif->get_command(worker->qxl, &ext_cmd)) {
>              *ring_is_empty = TRUE;
>              if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
> -                worker->display_poll_tries++;
>                  worker->event_timeout = MIN(worker->event_timeout,
> CMD_RING_POLL_TIMEOUT);
> -                return n;
> +            } else if (worker->display_poll_tries == CMD_RING_POLL_RETRIES &&
> +                       !worker->qxl->st->qif->req_cmd_notification(worker
> ->qxl)) {
> +                continue;
>              }
> -            if (worker->display_poll_tries > CMD_RING_POLL_RETRIES ||
> -                         worker->qxl->st->qif->req_cmd_notification(worker
> ->qxl)) {
> -                worker->display_poll_tries++;
> -                return n;
> -            }
> -            continue;
> +            worker->display_poll_tries++;
> +            return n;
>          }
>  
>          if (worker->record_fd)