Re: [Qemu-devel] [PATCH] make screendump an async command

[Alon Levy <alevy@redhat.com>]

> Il 13/06/2013 15:27, Alon Levy ha scritto:
> > This fixes the broken screendump behavior for qxl devices in native mode
> > since 81fb6f1504fb9ef71f2382f44af34756668296e8.
> > 
> > Note: due to QAPI not generating async commands yet I had to remove the
> > schema screendump definition.
> > 
> > Related RHBZ: 973374
> > This patch is not enough to fix said bz, with the linux qxl driver you get
> > garbage still, just not out of date garbage.
> > 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  hmp.c                     |  2 +-
> >  hw/display/qxl-render.c   |  1 +
> >  hw/display/vga.c          |  1 +
> >  include/qapi/qmp/qerror.h |  6 +++++
> >  include/ui/console.h      | 10 ++++++++
> >  qapi-schema.json          | 13 -----------
> >  qmp-commands.hx           |  3 ++-
> >  ui/console.c              | 58
> >  ++++++++++++++++++++++++++++++++++++++++++++++-
> >  8 files changed, 78 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 494a9aa..2a37975 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict
> > *qdict)
> >      const char *filename = qdict_get_str(qdict, "filename");
> >      Error *err = NULL;
> >  
> > -    qmp_screendump(filename, &err);
> > +    hmp_screen_dump_helper(filename, &err);
> >      hmp_handle_error(mon, &err);
> >  }
> >  
> > diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> > index f511a62..d719448 100644
> > --- a/hw/display/qxl-render.c
> > +++ b/hw/display/qxl-render.c
> > @@ -139,6 +139,7 @@ static void
> > qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
> >                         qxl->dirty[i].bottom - qxl->dirty[i].top);
> >      }
> >      qxl->num_dirty_rects = 0;
> > +    console_screendump_complete(vga->con);
> >  }
> >  
> >  /*
> > diff --git a/hw/display/vga.c b/hw/display/vga.c
> > index 21a108d..1fc52eb 100644
> > --- a/hw/display/vga.c
> > +++ b/hw/display/vga.c
> > @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
> >              break;
> >          }
> >      }
> > +    console_screendump_complete(s->con);
> >  }
> >  
> >  /* force a full display refresh */
> > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> > index 6c0a18d..1bd7efa 100644
> > --- a/include/qapi/qmp/qerror.h
> > +++ b/include/qapi/qmp/qerror.h
> > @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
> >  #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
> >      ERROR_CLASS_GENERIC_ERROR, "Migration is disabled when VirtFS export
> >      path '%s' is mounted in the guest using mount_tag '%s'"
> >  
> > +#define QERR_SCREENDUMP_NOT_AVAILABLE \
> > +    ERROR_CLASS_GENERIC_ERROR, "There is no QemuConsole I can screendump
> > from"
> 
> s/QemuConsole/graphical console/
> 
> > +#define QERR_SCREENDUMP_IN_PROGRESS \
> > +    ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress"
> > +
> 
> Please use error_setg instead.

I'm not sure what you are proposing. Something like this?

diff --git a/ui/console.c b/ui/console.c
index 0efd588..3a1e6ed 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -334,7 +334,9 @@ int qmp_screendump(Monitor *mon, const QDict *qdict,
         return -1;
     }
     if (con->screendump.filename != NULL) {
-        qerror_report(QERR_SCREENDUMP_IN_PROGRESS);
+        QError *err = NULL;
+        error_setg(err, "Existing screendump in progress");
+        monitor_set_error(mon, err);
         return -1;
     }
     con->screendump.filename = g_strdup(filename);

(minus defining err in the inner scope)


> 
> >  #define QERR_SOCKET_CONNECT_FAILED \
> >      ERROR_CLASS_GENERIC_ERROR, "Failed to connect to socket"
> >  
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 4307b5f..643da45 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -341,4 +341,14 @@ int index_from_keycode(int code);
> >  void early_gtk_display_init(void);
> >  void gtk_display_init(DisplayState *ds);
> >  
> > +/* hw/display/vga.c hw/display/qxl.c */
> > +void console_screendump_complete(QemuConsole *con);
> > +
> > +/* monitor.c */
> > +int qmp_screendump(Monitor *mon, const QDict *qdict,
> > +                   MonitorCompletion cb, void *opaque);
> > +
> > +/* hmp.c */
> > +void hmp_screen_dump_helper(const char *filename, Error **errp);
> > +
> >  #endif
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5ad6894..f5fdc2f 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3112,19 +3112,6 @@
> >    'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
> >  
> >  ##
> > -# @screendump:
> > -#
> > -# Write a PPM of the VGA screen to a file.
> > -#
> > -# @filename: the path of a new PPM file to store the image
> > -#
> > -# Returns: Nothing on success
> > -#
> > -# Since: 0.14.0
> > -##
> > -{ 'command': 'screendump', 'data': {'filename': 'str'} }
> 
> Please just comment out the declaration with an explanation.
> 
> Paolo
> 
> > -
> > -##
> >  # @nbd-server-start:
> >  #
> >  # Start an NBD server listening on the given host and port.  Block
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 8cea5e5..bde8f43 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -146,7 +146,8 @@ EQMP
> >      {
> >          .name       = "screendump",
> >          .args_type  = "filename:F",
> > -        .mhandler.cmd_new = qmp_marshal_input_screendump,
> > +        .mhandler.cmd_async = qmp_screendump,
> > +        .flags      = MONITOR_CMD_ASYNC,
> >      },
> >  
> >  SQMP
> > diff --git a/ui/console.c b/ui/console.c
> > index 28bba6d..0efd588 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -116,6 +116,12 @@ typedef enum {
> >  struct QemuConsole {
> >      Object parent;
> >  
> > +    struct {
> > +        char *filename;
> > +        MonitorCompletion *completion_cb;
> > +        void *completion_opaque;
> > +    } screendump;
> > +
> >      int index;
> >      console_type_t console_type;
> >      DisplayState *ds;
> > @@ -313,11 +319,61 @@ write_err:
> >      goto out;
> >  }
> >  
> > -void qmp_screendump(const char *filename, Error **errp)
> > +int qmp_screendump(Monitor *mon, const QDict *qdict,
> > +                   MonitorCompletion cb, void *opaque)
> >  {
> >      QemuConsole *con = qemu_console_lookup_by_index(0);
> > +    const char *filename = qdict_get_str(qdict, "filename");
> > +
> > +    if (con == NULL) {
> > +        qerror_report(QERR_SCREENDUMP_NOT_AVAILABLE);
> > +        return -1;
> > +    }
> > +    if (filename == NULL) {
> > +        qerror_report(QERR_MISSING_PARAMETER, "filename");
> > +        return -1;
> > +    }
> > +    if (con->screendump.filename != NULL) {
> > +        qerror_report(QERR_SCREENDUMP_IN_PROGRESS);
> > +        return -1;
> > +    }
> > +    con->screendump.filename = g_strdup(filename);
> > +    con->screendump.completion_cb = cb;
> > +    con->screendump.completion_opaque = opaque;
> > +
> > +    graphic_hw_update(con);
> > +    return 0;
> > +}
> > +
> > +void console_screendump_complete(QemuConsole *con)
> > +{
> > +    Error *errp = NULL;
> >      DisplaySurface *surface;
> >  
> > +    if (!con->screendump.filename) {
> > +        return;
> > +    }
> > +    assert(con->screendump.completion_cb);
> > +    surface = qemu_console_surface(con);
> > +    ppm_save(con->screendump.filename, surface, &errp);
> > +    if (errp) {
> > +        /*
> > +         * TODO: return data? raise error somehow? read qmp-spec for async
> > +         * error reporting.
> > +         */
> > +    }
> > +    con->screendump.completion_cb(con->screendump.completion_opaque,
> > NULL);
> > +    g_free(con->screendump.filename);
> > +    con->screendump.filename = NULL;
> > +    con->screendump.completion_cb = NULL;
> > +    con->screendump.completion_opaque = NULL;
> > +}
> > +
> > +void hmp_screen_dump_helper(const char *filename, Error **errp)
> > +{
> > +    DisplaySurface *surface;
> > +    QemuConsole *con = qemu_console_lookup_by_index(0);
> > +
> >      if (con == NULL) {
> >          error_setg(errp, "There is no QemuConsole I can screendump
> >          from.");
> >          return;
> > 
> 
> 
>