[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] qxl: switch qxl.c to trace-events
From: |
Alon Levy |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] qxl: switch qxl.c to trace-events |
Date: |
Wed, 14 Mar 2012 12:58:34 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Mar 14, 2012 at 10:03:06AM +0100, Gerd Hoffmann wrote:
> 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.
OK.
>
> > @@ -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?
No. So I guess I'll remove the dprint as well (in a separate patch).
>
> > 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.
No problem.
>
> > --- 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.
hmm, ok - one point is that I was conflicted about renaming some of the
trace points to have more namespace-y names, like
trace_qemu_spice_surface_primary_create
instead of
trace_qemu_spice_create_primary_surface
but opted in the end to stay with the later since they match the
function names they are tracing, and there is no autocomplete yet for
trace-events anyway, so globing is the only possible issue and it isn't
affected (as long as you remember to put enough asterisks).
>
> cheers,
> Gerd
>