[PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[Li Qiang]

Just use 'while (true)' to avoid duplicated.
No function change.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/display/virtio-gpu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f0dd7c150..9cef313f5e 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -869,13 +869,15 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     }
 #endif
 
-    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
-    while (cmd) {
+    while (true) {
+        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
+        if (!cmd) {
+            break;
+        }
         cmd->vq = vq;
         cmd->error = 0;
         cmd->finished = false;
         QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
-        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
     }
 
     virtio_gpu_process_cmdq(g);
-- 
2.17.1

Re: [PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[Laszlo Ersek]

On 08/13/20 17:36, Li Qiang wrote:
> Just use 'while (true)' to avoid duplicated.
> No function change.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/display/virtio-gpu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f0dd7c150..9cef313f5e 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -869,13 +869,15 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>      }
>  #endif
>  
> -    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> -    while (cmd) {
> +    while (true) {
> +        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> +        if (!cmd) {
> +            break;
> +        }
>          cmd->vq = vq;
>          cmd->error = 0;
>          cmd->finished = false;
>          QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
> -        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
>      }
>  
>      virtio_gpu_process_cmdq(g);
> 

There are (at least) three styles:

(1)

    thing = get_next();
    while (is_valid(thing)) {
        ...
        thing = get_next();
    }

(2)

    while (true) {
        thing = get_next();
        if (!is_valid(thing)) {
            break;
        }
        ...
     }

(3)

    while (is_valid(thing = get_next())) {
        ...
    }

My opinion:

- If the get_next() invocation is simple, then style (1) is perfectly fine.

- Style (2) is the worst of all.

- If style (1) is not appropriate for whatever reason, then style (3) is frequently a good replacement. Style (3) is sometimes rejected by coding style documents though. Style (3) is not usable if is_valid() is a function-like macro that does not evaluate its argument exactly once. Frequently, is_valid() is simply open-coded with C operators (using extra parens), for example:

    while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command)))) {

or more verbosely

    while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command))) !=
           NULL) {

If we really dislike style (1), then I'd propose style (3). I think the present patch (style (2)) is a step back.

Just my opinion of course; I don't feel too strongly about this.

Laszlo

Re: [PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[Li Qiang]

Laszlo Ersek <lersek@redhat.com> 于2020年8月14日周五 上午4:21写道：
>
> On 08/13/20 17:36, Li Qiang wrote:
> > Just use 'while (true)' to avoid duplicated.
> > No function change.
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  hw/display/virtio-gpu.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 5f0dd7c150..9cef313f5e 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -869,13 +869,15 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> >      }
> >  #endif
> >
> > -    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> > -    while (cmd) {
> > +    while (true) {
> > +        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> > +        if (!cmd) {
> > +            break;
> > +        }
> >          cmd->vq = vq;
> >          cmd->error = 0;
> >          cmd->finished = false;
> >          QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
> > -        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> >      }
> >
> >      virtio_gpu_process_cmdq(g);
> >
>
> There are (at least) three styles:
>
> (1)
>
>     thing = get_next();
>     while (is_valid(thing)) {
>         ...
>         thing = get_next();
>     }
>
> (2)
>
>     while (true) {
>         thing = get_next();
>         if (!is_valid(thing)) {
>             break;
>         }
>         ...
>      }
>
> (3)
>
>     while (is_valid(thing = get_next())) {
>         ...
>     }
>
> My opinion:
>
> - If the get_next() invocation is simple, then style (1) is perfectly fine.
>
> - Style (2) is the worst of all.
>
> - If style (1) is not appropriate for whatever reason, then style (3) is frequently a good replacement. Style (3) is sometimes rejected by coding style documents though. Style (3) is not usable if is_valid() is a function-like macro that does not evaluate its argument exactly once. Frequently, is_valid() is simply open-coded with C operators (using extra parens), for example:
>
>     while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command)))) {
>
> or more verbosely
>
>     while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command))) !=
>            NULL) {
>
> If we really dislike style (1), then I'd propose style (3). I think the present patch (style (2)) is a step back.
>

I have no strong opinion about the style(2) and style(3), just don't
like the dup of style(1).

Anyway, let Gerd do the choice. AFAICS, the qemu uses a lot of
stype(2)  to populate virtio requests.

Thanks,
Li Qiang

> Just my opinion of course; I don't feel too strongly about this.
>
> Laszlo
>

Re: [PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[Gerd Hoffmann]

  Hi,

> - If the get_next() invocation is simple, then style (1) is perfectly fine.

Fully agree.  Duplicating a single line is perfectly fine if it is
simple enough that you can hardly get it wrong.

> - Style (2) is the worst of all.

Yes, especially because the break is three lines not two due to qemu
code style.

> - If style (1) is not appropriate for whatever reason, then style (3) is frequently a good replacement. Style (3) is sometimes rejected by coding style documents though. Style (3) is not usable if is_valid() is a function-like macro that does not evaluate its argument exactly once. Frequently, is_valid() is simply open-coded with C operators (using extra parens), for example:

> or more verbosely
> 
>     while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command))) !=
>            NULL) {

Style 3 tends to generate long lines, which in turn need line breaks
like this.  Also many parens don't help making code more readable.
So I'd prefer to just leave things as-is.

take care,
  Gerd