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 >