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

[Li Qiang]

Laszlo Ersek <ler...@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 <liq...@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
>