Re: [Qemu-devel] [PATCH v2 1/2] qxl: switch qxl.c to trace-events

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> dprint is still used for qxl_init_common one time prints.
> 
> also switched parts of spice-display.c over, mainly all the callbacks to
> spice server.
> 
> All qxl device trace events start with the qxl device id.

Looks pretty good overall, just some little nits left ...


> @@ -948,8 +969,9 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>      VGACommonState *vga = opaque;
>      PCIQXLDevice *qxl = container_of(vga, PCIQXLDevice, vga);
>  
> +    trace_qxl_io_write_vga(qxl->id, qxl_mode_to_string(qxl->mode), addr, val);

With this being added and logging qxl->mode ...

>      if (qxl->mode != QXL_MODE_VGA) {
> -        dprint(qxl, 1, "%s\n", __FUNCTION__);
> +        trace_qxl_vga_ioport_while_not_in_vga_mode(qxl->id);

... this one isn't needed any more.

> @@ -1445,23 +1448,24 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>          qxl_update_irq(d);
>      } else {
>          if (write(d->pipe[1], d, 1) != 1) {
> -            dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
> +            trace_qxl_send_events_write_to_pipe_failed(d->id);

I somehow feel like we can just drop this.  kernel pipe buffers are 4096
at least, we are writing a single byte only, it is highly unlikely this
ever happens and even it if does we just ignore the error anyway.

Have you ever seen that message in a log?

>  static void init_pipe_signaling(PCIQXLDevice *d)
>  {
> -   if (pipe(d->pipe) < 0) {
> -       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
> -       return;
> -   }
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> -   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> -   fcntl(d->pipe[0], F_SETOWN, getpid());
> +    if (pipe(d->pipe) < 0) {
> +        fprintf(stderr, "%s:%s: qxl pipe creation failed\n",
> +                __FILE__, __func__);
> +        exit(1);
> +    }
> +    fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> +    fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> +    fcntl(d->pipe[0], F_SETOWN, getpid());
>  
> -   qemu_thread_get_self(&d->main);
> -   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
> +    qemu_thread_get_self(&d->main);
> +    qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
>  }

Intention fixup, would be nice as separate patch.

> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c

> +    trace_qxl_spice_add_memslot(ssd->qxl.id, memslot->slot_id,
> +                                memslot->virt_start, memslot->virt_end,
> +                                async);
> +

> +    trace_qxl_spice_del_memslot(ssd->qxl.id, gid, sid);

> +    trace_qemu_spice_create_primary_surface(ssd->qxl.id, id, surface, async);

> +    trace_qemu_spice_destroy_primary_surface(ssd->qxl.id, id, async);

I think these should all be qemu_spice_* for consistency.

cheers,
  Gerd