Hi On Fri, Nov 25, 2022 at 7:41 PM Philippe Mathieu-Daudé <[email protected]> wrote: > > Only 3 command types are logged: no need to call qxl_phys2virt() > for the other types. > > Signed-off-by: Philippe Mathieu-Daudé <[email protected]> > --- > hw/display/qxl-logger.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c > index 68bfa47568..1bcf803db6 100644 > --- a/hw/display/qxl-logger.c > +++ b/hw/display/qxl-logger.c > @@ -247,6 +247,16 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, > QXLCommandExt *ext) > qxl_name(qxl_type, ext->cmd.type), > compat ? "(compat)" : ""); > > + switch (ext->cmd.type) { > + case QXL_CMD_DRAW: > + break; > + case QXL_CMD_SURFACE: > + break; > + case QXL_CMD_CURSOR: > + break; > + default: > + goto out; > + }
That's a quite verbose way to repeat the case list below. Furthermore, it shouldn't hurt to call qxl_phys2virt() next. > data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id); But I understand better the change looking at patch 3, where you introduce the size argument. Maybe mention the coming change in the commit message? Reviewed-by: Marc-André Lureau <[email protected]> > if (!data) { > return 1; > @@ -269,6 +279,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, > QXLCommandExt *ext) > qxl_log_cmd_cursor(qxl, data, ext->group_id); > break; > } > +out: > fprintf(stderr, "\n"); > return 0; > } > -- > 2.38.1 > > -- Marc-André Lureau
