From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7k7N-0007O8-8V for qemu-devel@nongnu.org; Wed, 14 Mar 2012 05:03:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7k6y-0001XW-J2 for qemu-devel@nongnu.org; Wed, 14 Mar 2012 05:03:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34149) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7k6y-0001Wk-BB for qemu-devel@nongnu.org; Wed, 14 Mar 2012 05:03:12 -0400 Message-ID: <4F605ECA.1060200@redhat.com> Date: Wed, 14 Mar 2012 10:03:06 +0100 From: Gerd Hoffmann MIME-Version: 1.0 References: <1331662001-2733-1-git-send-email-alevy@redhat.com> <1331662001-2733-2-git-send-email-alevy@redhat.com> In-Reply-To: <1331662001-2733-2-git-send-email-alevy@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] qxl: switch qxl.c to trace-events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.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