[Qemu-devel] [PATCH qmp-next 00/14]: qapi: convert screendump

[Qemu-devel] [PATCH qmp-next 00/14]: qapi: convert screendump

[Luiz Capitulino]

Converting the screendump command is simple and shouldn't take more than
or or two patches, the complicated part is to report all errors correctly.

I hope I didn't go too far there, but at least this series does the right
thing (or is very near to).

 console.c        |   7 ++--
 console.h        |   5 +--
 cutils.c         | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx  |   5 ++-
 hmp.c            |   9 +++++
 hmp.h            |   1 +
 hw/blizzard.c    |   4 +--
 hw/g364fb.c      |  51 ++++++++++++++++++++--------
 hw/omap_lcdc.c   |  60 +++++++++++++++++++++++----------
 hw/qxl.c         |   7 ++--
 hw/tcx.c         |  96 ++++++++++++++++++++++++++++++++++++++++------------
 hw/vga.c         |  38 +++++++++++++--------
 hw/vga_int.h     |   3 +-
 hw/vmware_vga.c  |   7 ++--
 monitor.c        |   6 ----
 qapi-schema.json |  24 +++++++++++++
 qemu-common.h    |   7 ++++
 qerror.c         |  28 ++++++++++++++--
 qerror.h         |  22 ++++++++++--
 qmp-commands.hx  |   5 +--
 20 files changed, 390 insertions(+), 95 deletions(-)

[Qemu-devel] [PATCH 01/14] qerror: extend QERR_TOO_MANY_FILES

[Luiz Capitulino]

Specify it's too many open files in the system (ENFILE).

There's no compatibility problem because it's not used anywhere today.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c | 4 ++--
 qerror.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qerror.c b/qerror.c
index 5092fe7..2c97382 100644
--- a/qerror.c
+++ b/qerror.c
@@ -279,8 +279,8 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
-        .error_fmt = QERR_TOO_MANY_FILES,
-        .desc      = "Too many open files",
+        .error_fmt = QERR_TOO_MANY_FILES_SYS,
+        .desc      = "Too many opened files in the system",
     },
     {
         .error_fmt = QERR_UNDEFINED_ERROR,
diff --git a/qerror.h b/qerror.h
index 4cbba48..9ddf09c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -230,8 +230,8 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-#define QERR_TOO_MANY_FILES \
-    "{ 'class': 'TooManyFiles', 'data': {} }"
+#define QERR_TOO_MANY_FILES_SYS \
+    "{ 'class': 'TooManyFilesInSystem', 'data': {} }"
 
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 02/14] qerror: add new errors

[Luiz Capitulino]

New errors for write() and open() failures. Will be used by the
next commits.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c | 24 ++++++++++++++++++++++++
 qerror.h | 18 ++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/qerror.c b/qerror.c
index 2c97382..58e4570 100644
--- a/qerror.c
+++ b/qerror.c
@@ -152,6 +152,14 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "The feature '%(name)' is not enabled",
     },
     {
+        .error_fmt = QERR_FILE_TOO_BIG,
+        .desc      = "File exceeds maxium file size limit",
+    },
+    {
+        .error_fmt = QERR_INVALID_ACCESS,
+        .desc      = "The access is invalid",
+    },
+    {
         .error_fmt = QERR_INVALID_BLOCK_FORMAT,
         .desc      = "Invalid block format '%(name)'",
     },
@@ -209,10 +217,18 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Parameter '%(name)' is missing",
     },
     {
+        .error_fmt = QERR_NAME_TOO_LONG,
+        .desc      = "Name is too longe",
+    },
+    {
         .error_fmt = QERR_NO_BUS_FOR_DEVICE,
         .desc      = "No '%(bus)' bus found for device '%(device)'",
     },
     {
+        .error_fmt = QERR_NO_SPACE,
+        .desc      = "No space left in device",
+    },
+    {
         .error_fmt = QERR_NOT_SUPPORTED,
         .desc      = "Not supported",
     },
@@ -271,6 +287,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "QMP input object member '%(member)' is unexpected",
     },
     {
+        .error_fmt = QERR_READ_ONLY_FS,
+        .desc      = "File System is read-only",
+    },
+    {
         .error_fmt = QERR_RESET_REQUIRED,
         .desc      = "Resetting the Virtual Machine is required",
     },
@@ -279,6 +299,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_TOO_MANY_FILES_PROC,
+        .desc      = "Too many opened files by the process",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES_SYS,
         .desc      = "Too many opened files in the system",
     },
diff --git a/qerror.h b/qerror.h
index 9ddf09c..e8dc9e7 100644
--- a/qerror.h
+++ b/qerror.h
@@ -136,6 +136,12 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_FEATURE_DISABLED \
     "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
 
+#define QERR_FILE_TOO_BIG \
+    "{ 'class': 'FileTooBig', 'data': {} }"
+
+#define QERR_INVALID_ACCESS \
+    "{ 'class': 'InvalidAccess', 'data': {} }"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
@@ -178,9 +184,15 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_MISSING_PARAMETER \
     "{ 'class': 'MissingParameter', 'data': { 'name': %s } }"
 
+#define QERR_NAME_TOO_LONG \
+    "{ 'class': 'NameTooLong', 'data': {} }"
+
 #define QERR_NO_BUS_FOR_DEVICE \
     "{ 'class': 'NoBusForDevice', 'data': { 'device': %s, 'bus': %s } }"
 
+#define QERR_NO_SPACE \
+    "{ 'class': 'NoSpace', 'data': {} }"
+
 #define QERR_NOT_SUPPORTED \
     "{ 'class': 'NotSupported', 'data': {} }"
 
@@ -224,12 +236,18 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_EXTRA_MEMBER \
     "{ 'class': 'QMPExtraInputObjectMember', 'data': { 'member': %s } }"
 
+#define QERR_READ_ONLY_FS \
+    "{ 'class': 'ReadOnlyFileSystem', 'data': {} }"
+
 #define QERR_RESET_REQUIRED \
     "{ 'class': 'ResetRequired', 'data': {} }"
 
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_TOO_MANY_FILES_PROC \
+    "{ 'class': 'TooManyFilesByProcess', 'data': {} }"
+
 #define QERR_TOO_MANY_FILES_SYS \
     "{ 'class': 'TooManyFilesInSystem', 'data': {} }"
 
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 03/14] cutils: introduce qemu_fopen_err()

[Luiz Capitulino]

A fopen() wrapper that takes an Error argument.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cutils.c      | 42 ++++++++++++++++++++++++++++++++++++++++++
 qemu-common.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/cutils.c b/cutils.c
index af308cd..bdee130 100644
--- a/cutils.c
+++ b/cutils.c
@@ -26,6 +26,7 @@
 #include <math.h>
 
 #include "qemu_socket.h"
+#include "error.h"
 
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
@@ -549,3 +550,44 @@ int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset)
     return do_sendv_recvv(sockfd, iov, len, iov_offset, 1);
 }
 
+static void set_open_err(Error **errp, const char *path, int err_nr)
+{
+    switch (err_nr) {
+    case EACCES:
+        error_set(errp, QERR_INVALID_ACCESS);
+        return;
+    case EMFILE:
+        error_set(errp, QERR_TOO_MANY_FILES_PROC);
+        return;
+    case ENFILE:
+        error_set(errp, QERR_TOO_MANY_FILES_SYS);
+        return;
+    case ENAMETOOLONG:
+        error_set(errp, QERR_NAME_TOO_LONG);
+        return;
+    case ENOSPC:
+        error_set(errp, QERR_NO_SPACE);
+        return;
+    case EPERM:
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    case EROFS:
+        error_set(errp, QERR_READ_ONLY_FS);
+        return;
+    default:
+        error_set(errp, QERR_OPEN_FILE_FAILED, path);
+        return;
+    }
+}
+
+FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp)
+{
+    FILE *fp;
+
+    fp = fopen(path, mode);
+    if (!fp) {
+        set_open_err(errp, path, errno);
+    }
+
+    return fp;
+}
diff --git a/qemu-common.h b/qemu-common.h
index cccfb42..2cab2a8 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -4,6 +4,7 @@
 
 #include "compiler.h"
 #include "config-host.h"
+#include "error.h"
 
 #if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
 #define WORDS_ALIGNED
@@ -208,6 +209,8 @@ int qemu_pipe(int pipefd[2]);
 int qemu_recvv(int sockfd, struct iovec *iov, int len, int iov_offset);
 int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset);
 
+FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp);
+
 /* Error handling.  */
 
 void QEMU_NORETURN hw_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 04/14] cutils: introduce qemu_fprintf_err()

[Luiz Capitulino]

A fprintf() wrapper that takes an Error argument.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cutils.c      | 31 +++++++++++++++++++++++++++++++
 qemu-common.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/cutils.c b/cutils.c
index bdee130..f5b02b4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -591,3 +591,34 @@ FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp)
 
     return fp;
 }
+
+static void set_write_err(Error **errp, int err_nr)
+{
+    switch (err_nr) {
+    case ENOSPC:
+        error_set(errp, QERR_NO_SPACE);
+        return;
+    case EFBIG:
+        error_set(errp, QERR_FILE_TOO_BIG);
+        return;
+    default:
+        error_set(errp, QERR_IO_ERROR);
+        return;
+    }
+}
+
+int qemu_fprintf_err(Error **errp, FILE *stream, const char *format, ...)
+{
+    va_list ap;
+    int ret;
+
+    va_start(ap, format);
+    ret = vfprintf(stream, format, ap);
+    va_end(ap);
+
+    if (ret < 0) {
+        set_write_err(errp, errno);
+    }
+
+    return ret;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 2cab2a8..58c5197 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -210,6 +210,7 @@ int qemu_recvv(int sockfd, struct iovec *iov, int len, int iov_offset);
 int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset);
 
 FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp);
+int qemu_fprintf_err(Error **errp, FILE *stream, const char *format, ...);
 
 /* Error handling.  */
 
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 05/14] cutils: introduce qemu_fputc_err()

[Luiz Capitulino]

A fputc() wrapper that takes an Error argument.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cutils.c      | 12 ++++++++++++
 qemu-common.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/cutils.c b/cutils.c
index f5b02b4..83edb43 100644
--- a/cutils.c
+++ b/cutils.c
@@ -622,3 +622,15 @@ int qemu_fprintf_err(Error **errp, FILE *stream, const char *format, ...)
 
     return ret;
 }
+
+int qemu_fputc_err(int c, FILE *stream, Error **errp)
+{
+    int ret;
+
+    ret = fputc(c, stream);
+    if (ret == EOF) {
+        set_write_err(errp, errno);
+    }
+
+    return ret;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 58c5197..895bc2b 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -211,6 +211,7 @@ int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset);
 
 FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp);
 int qemu_fprintf_err(Error **errp, FILE *stream, const char *format, ...);
+int qemu_fputc_err(int c, FILE *stream, Error **errp);
 
 /* Error handling.  */
 
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 06/14] cutils: introduce qemu_fwrite_err()

[Luiz Capitulino]

A fwrite() wrapper that takes an Error argument.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cutils.c      | 15 +++++++++++++++
 qemu-common.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/cutils.c b/cutils.c
index 83edb43..63fdfc0 100644
--- a/cutils.c
+++ b/cutils.c
@@ -634,3 +634,18 @@ int qemu_fputc_err(int c, FILE *stream, Error **errp)
 
     return ret;
 }
+
+size_t qemu_fwrite_err(const void *ptr, size_t size, size_t nmemb,
+                       FILE *stream, Error **errp)
+{
+    size_t ret;
+
+    clearerr(stream);
+    ret = fwrite(ptr, size, nmemb, stream);
+
+    if (ferror(stream)) {
+        set_write_err(errp, errno);
+    }
+
+    return ret;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 895bc2b..39377ff 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -212,6 +212,8 @@ int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset);
 FILE *qemu_fopen_err(const char *path, const char *mode, Error **errp);
 int qemu_fprintf_err(Error **errp, FILE *stream, const char *format, ...);
 int qemu_fputc_err(int c, FILE *stream, Error **errp);
+size_t qemu_fwrite_err(const void *ptr, size_t size, size_t nmemb,
+                       FILE *stream, Error **errp);
 
 /* Error handling.  */
 
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 07/14] omap_lcdc: rename ppm_save() to omap_ppm_save()

[Luiz Capitulino]

Avoids confusion with the global ppm_save() defined in hw/vga.c.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/omap_lcdc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index 4a08e9d..6d2e83a 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -224,8 +224,8 @@ static void omap_update_display(void *opaque)
     omap_lcd->invalidate = 0;
 }
 
-static int ppm_save(const char *filename, uint8_t *data,
-                int w, int h, int linesize)
+static int omap_ppm_save(const char *filename, uint8_t *data,
+                    int w, int h, int linesize)
 {
     FILE *f;
     uint8_t *d, *d1;
@@ -270,9 +270,9 @@ static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
 
     omap_update_display(opaque);
     if (omap_lcd && ds_get_data(omap_lcd->state))
-        ppm_save(filename, ds_get_data(omap_lcd->state),
-                omap_lcd->width, omap_lcd->height,
-                ds_get_linesize(omap_lcd->state));
+        omap_ppm_save(filename, ds_get_data(omap_lcd->state),
+                    omap_lcd->width, omap_lcd->height,
+                    ds_get_linesize(omap_lcd->state));
 }
 
 static void omap_invalidate_display(void *opaque) {
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 08/14] console: vga_hw_screen_dump_ptr: take an Error argument

[Luiz Capitulino]

The new argument is not used in this commit. Error handling will
be added to each device individually in later commits.

All devices that register a screen dump callback via
graphic_console_init() are updated.

This work is required by the future conversion of the screendump
command to the QAPI.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 console.c       |  2 +-
 console.h       |  4 +++-
 hw/blizzard.c   |  2 +-
 hw/g364fb.c     |  3 ++-
 hw/omap_lcdc.c  |  3 ++-
 hw/qxl.c        |  5 +++--
 hw/tcx.c        | 12 ++++++++----
 hw/vga.c        |  6 ++++--
 hw/vmware_vga.c |  5 +++--
 9 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/console.c b/console.c
index 6a463f5..4669e62 100644
--- a/console.c
+++ b/console.c
@@ -187,7 +187,7 @@ void vga_hw_screen_dump(const char *filename)
         console_select(0);
     }
     if (consoles[0] && consoles[0]->hw_screen_dump) {
-        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
+        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, NULL);
     } else {
         error_report("screen dump not implemented");
     }
diff --git a/console.h b/console.h
index 4334db5..9caeb43 100644
--- a/console.h
+++ b/console.h
@@ -6,6 +6,7 @@
 #include "notify.h"
 #include "monitor.h"
 #include "trace.h"
+#include "error.h"
 
 /* keyboard/mouse support */
 
@@ -343,7 +344,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 
 typedef void (*vga_hw_update_ptr)(void *);
 typedef void (*vga_hw_invalidate_ptr)(void *);
-typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch,
+                                       Error **errp);
 typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
 
 DisplayState *graphic_console_init(vga_hw_update_ptr update,
diff --git a/hw/blizzard.c b/hw/blizzard.c
index 29074c4..a2b9053 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -933,7 +933,7 @@ static void blizzard_update_display(void *opaque)
 }
 
 static void blizzard_screen_dump(void *opaque, const char *filename,
-                                 bool cswitch)
+                                 bool cswitch, Error **errp)
 {
     BlizzardState *s = (BlizzardState *) opaque;
 
diff --git a/hw/g364fb.c b/hw/g364fb.c
index 3a0b68f..498154b 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -289,7 +289,8 @@ static void g364fb_reset(G364State *s)
     g364fb_invalidate_display(s);
 }
 
-static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
+                               Error **errp)
 {
     G364State *s = opaque;
     int y, x;
diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index 6d2e83a..3d6328f 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -264,7 +264,8 @@ static int omap_ppm_save(const char *filename, uint8_t *data,
     return 0;
 }
 
-static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
+                             Error **errp)
 {
     struct omap_lcd_panel_s *omap_lcd = opaque;
 
diff --git a/hw/qxl.c b/hw/qxl.c
index 3da3399..4dc0b7d 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1571,7 +1571,8 @@ static void qxl_hw_invalidate(void *opaque)
     vga->invalidate(vga);
 }
 
-static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
+                               Error **errp)
 {
     PCIQXLDevice *qxl = opaque;
     VGACommonState *vga = &qxl->vga;
@@ -1583,7 +1584,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
         ppm_save(filename, qxl->ssd.ds->surface);
         break;
     case QXL_MODE_VGA:
-        vga->screen_dump(vga, filename, cswitch);
+        vga->screen_dump(vga, filename, cswitch, errp);
         break;
     default:
         break;
diff --git a/hw/tcx.c b/hw/tcx.c
index ac7dcb4..74a7085 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -56,8 +56,10 @@ typedef struct TCXState {
     uint8_t dac_index, dac_state;
 } TCXState;
 
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch);
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+                            Error **errp);
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+                            Error **errp);
 
 static void tcx_set_dirty(TCXState *s)
 {
@@ -574,7 +576,8 @@ static int tcx_init1(SysBusDevice *dev)
     return 0;
 }
 
-static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
+                            Error **errp)
 {
     TCXState *s = opaque;
     FILE *f;
@@ -601,7 +604,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch)
     return;
 }
 
-static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
+                              Error **errp)
 {
     TCXState *s = opaque;
     FILE *f;
diff --git a/hw/vga.c b/hw/vga.c
index 5824f85..e6b154c 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -163,7 +163,8 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+                            Error **errp);
 
 static void vga_update_memory_access(VGACommonState *s)
 {
@@ -2413,7 +2414,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
+                            Error **errp)
 {
     VGACommonState *s = opaque;
 
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 142d9f4..171b1d9 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1003,11 +1003,12 @@ static void vmsvga_invalidate_display(void *opaque)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
+static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
+                               Error **errp)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-        s->vga.screen_dump(&s->vga, filename, cswitch);
+        s->vga.screen_dump(&s->vga, filename, cswitch, errp);
         return;
     }
 
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 09/14] vga: ppm_save(): add error handling

[Luiz Capitulino]

This is done by using qemu_fopen_err(), qemu_fwrite_err() and handling
errors appropriately (eg. removing the screendump file if the operation
fails).

Adding error handling to ppm_save() has the net effect of automatically
adding error handling for all devices using ppm_save(), but note that
the error is not passed up yet, as vga_hw_screen_dump() still calls
consoles[0]->hw_screen_dump() with errp=NULL.

The error will be propagated up when screendump is converted to the QAPI.
That will be done by a later commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/blizzard.c   |  2 +-
 hw/qxl.c        |  2 +-
 hw/vga.c        | 32 +++++++++++++++++++++-----------
 hw/vga_int.h    |  3 ++-
 hw/vmware_vga.c |  2 +-
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/blizzard.c b/hw/blizzard.c
index a2b9053..d1c9d81 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -939,7 +939,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
 
     blizzard_update_display(opaque);
     if (s && ds_get_data(s->state))
-        ppm_save(filename, s->state->surface);
+        ppm_save(filename, s->state->surface, errp);
 }
 
 #define DEPTH 8
diff --git a/hw/qxl.c b/hw/qxl.c
index 4dc0b7d..e32e517 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1581,7 +1581,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch,
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
         qxl_render_update(qxl);
-        ppm_save(filename, qxl->ssd.ds->surface);
+        ppm_save(filename, qxl->ssd.ds->surface, errp);
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename, cswitch, errp);
diff --git a/hw/vga.c b/hw/vga.c
index e6b154c..feb808f 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2368,22 +2368,25 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 /********************************************************/
 /* vga screen dump */
 
-int ppm_save(const char *filename, struct DisplaySurface *ds)
+void ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
 {
     FILE *f;
     uint8_t *d, *d1;
     uint32_t v;
     int y, x;
     uint8_t r, g, b;
-    int ret;
     char *linebuf, *pbuf;
 
     trace_ppm_save(filename, ds);
-    f = fopen(filename, "wb");
-    if (!f)
-        return -1;
-    fprintf(f, "P6\n%d %d\n%d\n",
-            ds->width, ds->height, 255);
+    f = qemu_fopen_err(filename, "wb", errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    qemu_fprintf_err(errp, f, "P6\n%d %d\n%d\n", ds->width, ds->height, 255);
+    if (error_is_set(errp)) {
+        linebuf = NULL;
+        goto out;
+    }
     linebuf = g_malloc(ds->width * 3);
     d1 = ds->data;
     for(y = 0; y < ds->height; y++) {
@@ -2404,12 +2407,19 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
             d += ds->pf.bytes_per_pixel;
         }
         d1 += ds->linesize;
-        ret = fwrite(linebuf, 1, pbuf - linebuf, f);
-        (void)ret;
+        qemu_fwrite_err(linebuf, 1, pbuf - linebuf, f, errp);
+        if (error_is_set(errp)) {
+            break;
+        }
     }
+
+out:
     g_free(linebuf);
     fclose(f);
-    return 0;
+
+    if (error_is_set(errp)) {
+        unlink(filename);
+    }
 }
 
 /* save the vga display in a PPM image even if no display is
@@ -2423,5 +2433,5 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
         vga_invalidate_display(s);
     }
     vga_hw_update();
-    ppm_save(filename, s->ds->surface);
+    ppm_save(filename, s->ds->surface, errp);
 }
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 7685b2b..7234cdf 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -23,6 +23,7 @@
  */
 
 #include <hw/hw.h>
+#include "error.h"
 #include "memory.h"
 
 #define ST01_V_RETRACE      0x08
@@ -200,7 +201,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val);
 uint32_t vga_mem_readb(VGACommonState *s, target_phys_addr_t addr);
 void vga_mem_writeb(VGACommonState *s, target_phys_addr_t addr, uint32_t val);
 void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
-int ppm_save(const char *filename, struct DisplaySurface *ds);
+void ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
 
 int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
 void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 171b1d9..4e68ea4 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1015,7 +1015,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch,
     if (s->depth == 32) {
         DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
                 s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
-        ppm_save(filename, ds);
+        ppm_save(filename, ds, errp);
         g_free(ds);
     }
 }
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 10/14] omap_lcdc: omap_ppm_save(): add error handling

[Luiz Capitulino]

This is done by using qemu_fopen_err(), qemu_fputc_err() and handling
errors appropriately (eg. removing the screendump file if the operation
fails).

Note that the error is not passed up yet, as vga_hw_screen_dump() still
calls consoles[0]->hw_screen_dump() with errp=NULL.

The error will be propagated up when screendump is converted to the QAPI.
That will be done by a later commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/omap_lcdc.c | 53 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index 3d6328f..cf5a264 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -224,18 +224,22 @@ static void omap_update_display(void *opaque)
     omap_lcd->invalidate = 0;
 }
 
-static int omap_ppm_save(const char *filename, uint8_t *data,
-                    int w, int h, int linesize)
+static void omap_ppm_save(const char *filename, uint8_t *data,
+                    int w, int h, int linesize, Error **errp)
 {
     FILE *f;
     uint8_t *d, *d1;
     unsigned int v;
     int y, x, bpp;
 
-    f = fopen(filename, "wb");
-    if (!f)
-        return -1;
-    fprintf(f, "P6\n%d %d\n%d\n", w, h, 255);
+    f = qemu_fopen_err(filename, "wb", errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    qemu_fprintf_err(errp, f, "P6\n%d %d\n%d\n", w, h, 255);
+    if (error_is_set(errp)) {
+        goto out;
+    }
     d1 = data;
     bpp = linesize / w;
     for (y = 0; y < h; y ++) {
@@ -244,24 +248,45 @@ static int omap_ppm_save(const char *filename, uint8_t *data,
             v = *(uint32_t *) d;
             switch (bpp) {
             case 2:
-                fputc((v >> 8) & 0xf8, f);
-                fputc((v >> 3) & 0xfc, f);
-                fputc((v << 3) & 0xf8, f);
+                qemu_fputc_err((v >> 8) & 0xf8, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+                qemu_fputc_err((v >> 3) & 0xfc, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+                qemu_fputc_err((v << 3) & 0xf8, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
                 break;
             case 3:
             case 4:
             default:
-                fputc((v >> 16) & 0xff, f);
-                fputc((v >> 8) & 0xff, f);
-                fputc((v) & 0xff, f);
+                qemu_fputc_err((v >> 16) & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+                qemu_fputc_err((v >> 8) & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+                qemu_fputc_err((v) & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
                 break;
             }
             d += bpp;
         }
         d1 += linesize;
     }
+out:
     fclose(f);
-    return 0;
+    if (error_is_set(errp)) {
+        unlink(filename);
+    }
 }
 
 static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
@@ -273,7 +298,7 @@ static void omap_screen_dump(void *opaque, const char *filename, bool cswitch,
     if (omap_lcd && ds_get_data(omap_lcd->state))
         omap_ppm_save(filename, ds_get_data(omap_lcd->state),
                     omap_lcd->width, omap_lcd->height,
-                    ds_get_linesize(omap_lcd->state));
+                    ds_get_linesize(omap_lcd->state), errp);
 }
 
 static void omap_invalidate_display(void *opaque) {
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 11/14] g364fb: g364fb_screen_dump(): add error handling

[Luiz Capitulino]

This is done by using qemu_fopen_err(), qemu_fputc_err() and handling
errors appropriately (eg. removing the screendump file if the operation
fails).

Note that the error is not passed up yet, as vga_hw_screen_dump() still
calls consoles[0]->hw_screen_dump() with errp=NULL.

The error will be propagated up when screendump is converted to the QAPI.
That will be done by a later commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/g364fb.c | 48 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/hw/g364fb.c b/hw/g364fb.c
index 498154b..a18a414 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -301,35 +301,59 @@ static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch,
     qemu_flush_coalesced_mmio_buffer();
 
     if (s->depth != 8) {
-        error_report("g364: unknown guest depth %d", s->depth);
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "depth", "8");
         return;
     }
 
-    f = fopen(filename, "wb");
-    if (!f)
+    f = qemu_fopen_err(filename, "wb", errp);
+    if (error_is_set(errp)) {
         return;
+    }
 
     if (s->ctla & CTLA_FORCE_BLANK) {
         /* blank screen */
-        fprintf(f, "P4\n%d %d\n",
-            s->width, s->height);
+        qemu_fprintf_err(errp, f, "P4\n%d %d\n", s->width, s->height);
+        if (error_is_set(errp)) {
+            goto out;
+        }
         for (y = 0; y < s->height; y++)
-            for (x = 0; x < s->width; x++)
-                fputc(0, f);
+            for (x = 0; x < s->width; x++) {
+                qemu_fputc_err(0, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+            }
     } else {
         data_buffer = s->vram + s->top_of_screen;
-        fprintf(f, "P6\n%d %d\n%d\n",
-            s->width, s->height, 255);
+        qemu_fprintf_err(errp, f, "P6\n%d %d\n%d\n", s->width, s->height, 255);
+        if (error_is_set(errp)) {
+            goto out;
+        }
         for (y = 0; y < s->height; y++)
             for (x = 0; x < s->width; x++, data_buffer++) {
                 index = *data_buffer;
-                fputc(s->color_palette[index][0], f);
-                fputc(s->color_palette[index][1], f);
-                fputc(s->color_palette[index][2], f);
+                qemu_fputc_err(s->color_palette[index][0], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err(s->color_palette[index][1], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err(s->color_palette[index][2], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
         }
     }
 
+out:
     fclose(f);
+    if (error_is_set(errp)) {
+        unlink(filename);
+    }
 }
 
 /* called for accesses to io ports */
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 12/14] tcx: tcx24_screen_dump(): add error handling

[Luiz Capitulino]

This is done by using qemu_fopen_err(), qemu_fputc_err() and handling
errors appropriately (eg. removing the screendump file if the operation
fails).

Note that the error is not passed up yet, as vga_hw_screen_dump() still
calls consoles[0]->hw_screen_dump() with errp=NULL.

The error will be propagated up when screendump is converted to the QAPI.
That will be done by a later commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/tcx.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/hw/tcx.c b/hw/tcx.c
index 74a7085..4027363 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -613,10 +613,14 @@ static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
     uint32_t *s24, *cptr, dval;
     int y, x;
 
-    f = fopen(filename, "wb");
-    if (!f)
+    f = qemu_fopen_err(filename, "wb", errp);
+    if (error_is_set(errp)) {
         return;
-    fprintf(f, "P6\n%d %d\n%d\n", s->width, s->height, 255);
+    }
+    qemu_fprintf_err(errp, f, "P6\n%d %d\n%d\n", s->width, s->height, 255);
+    if (error_is_set(errp)) {
+        goto out;
+    }
     d1 = s->vram;
     s24 = s->vram24;
     cptr = s->cplane;
@@ -625,20 +629,46 @@ static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
         for(x = 0; x < s->width; x++, d++, s24++) {
             if ((*cptr++ & 0xff000000) == 0x03000000) { // 24-bit direct
                 dval = *s24 & 0x00ffffff;
-                fputc((dval >> 16) & 0xff, f);
-                fputc((dval >> 8) & 0xff, f);
-                fputc(dval & 0xff, f);
+                qemu_fputc_err((dval >> 16) & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err((dval >> 8) & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err(dval & 0xff, f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
             } else {
                 v = *d;
-                fputc(s->r[v], f);
-                fputc(s->g[v], f);
-                fputc(s->b[v], f);
+                qemu_fputc_err(s->r[v], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err(s->g[v], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
+
+                qemu_fputc_err(s->b[v], f, errp);
+                if (error_is_set(errp)) {
+                    goto out;
+                }
             }
         }
         d1 += MAXX;
     }
+
+out:
     fclose(f);
-    return;
+    if (error_is_set(errp)) {
+        unlink(filename);
+    }
 }
 
 static Property tcx_properties[] = {
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 13/14] tcx: tcx_screen_dump(): add error handling

[Luiz Capitulino]

This is done by using qemu_fopen_err(), qemu_fputc_err() and handling
errors appropriately (eg. removing the screendump file if the operation
fails).

Note that the error is not passed up yet, as vga_hw_screen_dump() still
calls consoles[0]->hw_screen_dump() with errp=NULL.

The error will be propagated up when screendump is converted to the QAPI.
That will be done by a later commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/tcx.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/hw/tcx.c b/hw/tcx.c
index 4027363..7aa1abe 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -584,24 +584,44 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch,
     uint8_t *d, *d1, v;
     int y, x;
 
-    f = fopen(filename, "wb");
-    if (!f)
+    f = qemu_fopen_err(filename, "wb", errp);
+    if (error_is_set(errp)) {
         return;
-    fprintf(f, "P6\n%d %d\n%d\n", s->width, s->height, 255);
+    }
+    qemu_fprintf_err(errp, f, "P6\n%d %d\n%d\n", s->width, s->height, 255);
+    if (error_is_set(errp)) {
+        goto out;
+    }
     d1 = s->vram;
     for(y = 0; y < s->height; y++) {
         d = d1;
         for(x = 0; x < s->width; x++) {
             v = *d;
-            fputc(s->r[v], f);
-            fputc(s->g[v], f);
-            fputc(s->b[v], f);
+            qemu_fputc_err(s->r[v], f, errp);
+            if (error_is_set(errp)) {
+                goto out;
+            }
+
+            qemu_fputc_err(s->g[v], f, errp);
+            if (error_is_set(errp)) {
+                goto out;
+            }
+
+            qemu_fputc_err(s->b[v], f, errp);
+            if (error_is_set(errp)) {
+                goto out;
+            }
+
             d++;
         }
         d1 += MAXX;
     }
+
+out:
     fclose(f);
-    return;
+    if (error_is_set(errp)) {
+        unlink(filename);
+    }
 }
 
 static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch,
-- 
1.7.10.2.565.gbd578b5

[Qemu-devel] [PATCH 14/14] qapi: convert screendump

[Luiz Capitulino]

Also activates error reporting from devices.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 console.c        |  7 ++++---
 console.h        |  1 -
 hmp-commands.hx  |  5 ++---
 hmp.c            |  9 +++++++++
 hmp.h            |  1 +
 monitor.c        |  6 ------
 qapi-schema.json | 24 ++++++++++++++++++++++++
 qmp-commands.hx  |  5 +----
 8 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/console.c b/console.c
index 4669e62..2bbf104 100644
--- a/console.c
+++ b/console.c
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "console.h"
 #include "qemu-timer.h"
+#include "qmp-commands.h"
 
 //#define DEBUG_CONSOLE
 #define DEFAULT_BACKSCROLL 512
@@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
         active_console->hw_invalidate(active_console->hw);
 }
 
-void vga_hw_screen_dump(const char *filename)
+void qmp_screendump(const char *filename, Error **errp)
 {
     TextConsole *previous_active_console;
     bool cswitch;
@@ -187,9 +188,9 @@ void vga_hw_screen_dump(const char *filename)
         console_select(0);
     }
     if (consoles[0] && consoles[0]->hw_screen_dump) {
-        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, NULL);
+        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
     } else {
-        error_report("screen dump not implemented");
+        error_set(errp, QERR_NOT_SUPPORTED);
     }
 
     if (cswitch) {
diff --git a/console.h b/console.h
index 9caeb43..d72c546 100644
--- a/console.h
+++ b/console.h
@@ -356,7 +356,6 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
 
 void vga_hw_update(void);
 void vga_hw_invalidate(void);
-void vga_hw_screen_dump(const char *filename);
 void vga_hw_text_update(console_ch_t *chardata);
 
 int is_graphic_console(void);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 18cb415..74f61bc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -193,9 +193,8 @@ ETEXI
         .name       = "screendump",
         .args_type  = "filename:F",
         .params     = "filename",
-        .help       = "save screen into PPM image 'filename'",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_screen_dump,
+        .help       = "save screen into PPM image 'filepath'",
+        .mhandler.cmd = hmp_screen_dump,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index bb0952e..7e8c0ab 100644
--- a/hmp.c
+++ b/hmp.c
@@ -947,3 +947,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
     qmp_device_del(id, &err);
     hmp_handle_error(mon, &err);
 }
+
+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_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 443b812..fa75ec0 100644
--- a/hmp.h
+++ b/hmp.h
@@ -61,5 +61,6 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
+void hmp_screen_dump(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 12a6fe2..1160af7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -897,12 +897,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
     return -1;
 }
 
-static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
-    vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
-    return 0;
-}
-
 static void do_logfile(Monitor *mon, const QDict *qdict)
 {
     cpu_set_log_filename(qdict_get_str(qdict, "filename"));
diff --git a/qapi-schema.json b/qapi-schema.json
index 2ca7195..43d4acd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1755,3 +1755,27 @@
 # Since: 0.14.0
 ##
 { 'command': 'device_del', 'data': {'id': 'str'} }
+
+##
+# @screendump:
+#
+# Write a PPM of the VGA screen to a file.
+#
+# @filename: the path of a new PPM file to store the image
+#
+# Since: 0.14.0
+#
+# Returns: Nothing on success
+#   If the underlying device doesn't support screen dump, NotSupported
+#   If access to the file is not allowed, InvalidAccess
+#   If QEMU has too many open files, TooManyFilesByProcess
+#   If the system has too many open files, TooManyFilesInSystem
+#   If @filename is too long, NameTooLong
+#   If the system doesn't have enough space, NoSpace
+#   If the file-system to write the file to is read-only, ReadOnlyFileSystem
+#   If opening @filename fails for an uknown reason, OpenFileFailed
+#   If @filename becames too big, FileTooBig
+#   If an I/O happens while writing @filename, IOError
+#
+##
+{ 'command': 'screendump', 'data': {'filename': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db980fa..2a3e6d5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -146,10 +146,7 @@ EQMP
     {
         .name       = "screendump",
         .args_type  = "filename:F",
-        .params     = "filename",
-        .help       = "save screen into PPM image 'filename'",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_screen_dump,
+        .mhandler.cmd_new = qmp_marshal_input_screendump,
     },
 
 SQMP
-- 
1.7.10.2.565.gbd578b5

Re: [Qemu-devel] [PATCH 14/14] qapi: convert screendump

[Alon Levy]

On Fri, May 25, 2012 at 04:41:19PM -0300, Luiz Capitulino wrote:

One small note below.

> Also activates error reporting from devices.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  console.c        |  7 ++++---
>  console.h        |  1 -
>  hmp-commands.hx  |  5 ++---
>  hmp.c            |  9 +++++++++
>  hmp.h            |  1 +
>  monitor.c        |  6 ------
>  qapi-schema.json | 24 ++++++++++++++++++++++++
>  qmp-commands.hx  |  5 +----
>  8 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/console.c b/console.c
> index 4669e62..2bbf104 100644
> --- a/console.c
> +++ b/console.c
> @@ -24,6 +24,7 @@
>  #include "qemu-common.h"
>  #include "console.h"
>  #include "qemu-timer.h"
> +#include "qmp-commands.h"
>  
>  //#define DEBUG_CONSOLE
>  #define DEFAULT_BACKSCROLL 512
> @@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
>          active_console->hw_invalidate(active_console->hw);
>  }
>  
> -void vga_hw_screen_dump(const char *filename)
> +void qmp_screendump(const char *filename, Error **errp)
>  {
>      TextConsole *previous_active_console;
>      bool cswitch;
> @@ -187,9 +188,9 @@ void vga_hw_screen_dump(const char *filename)
>          console_select(0);
>      }
>      if (consoles[0] && consoles[0]->hw_screen_dump) {
> -        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, NULL);
> +        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
>      } else {
> -        error_report("screen dump not implemented");
> +        error_set(errp, QERR_NOT_SUPPORTED);
>      }
>  
>      if (cswitch) {
> diff --git a/console.h b/console.h
> index 9caeb43..d72c546 100644
> --- a/console.h
> +++ b/console.h
> @@ -356,7 +356,6 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>  
>  void vga_hw_update(void);
>  void vga_hw_invalidate(void);
> -void vga_hw_screen_dump(const char *filename);
>  void vga_hw_text_update(console_ch_t *chardata);
>  
>  int is_graphic_console(void);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 18cb415..74f61bc 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -193,9 +193,8 @@ ETEXI
>          .name       = "screendump",
>          .args_type  = "filename:F",
>          .params     = "filename",
> -        .help       = "save screen into PPM image 'filename'",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_screen_dump,
> +        .help       = "save screen into PPM image 'filepath'",
> +        .mhandler.cmd = hmp_screen_dump,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index bb0952e..7e8c0ab 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -947,3 +947,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>      qmp_device_del(id, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +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_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 443b812..fa75ec0 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -61,5 +61,6 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_device_del(Monitor *mon, const QDict *qdict);
> +void hmp_screen_dump(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..1160af7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -897,12 +897,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
>      return -1;
>  }
>  
> -static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> -    vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
> -    return 0;
> -}
> -
>  static void do_logfile(Monitor *mon, const QDict *qdict)
>  {
>      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2ca7195..43d4acd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1755,3 +1755,27 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'device_del', 'data': {'id': 'str'} }
> +
> +##
> +# @screendump:
> +#
> +# Write a PPM of the VGA screen to a file.
> +#
> +# @filename: the path of a new PPM file to store the image
> +#
> +# Since: 0.14.0
> +#
> +# Returns: Nothing on success
> +#   If the underlying device doesn't support screen dump, NotSupported
> +#   If access to the file is not allowed, InvalidAccess
> +#   If QEMU has too many open files, TooManyFilesByProcess
> +#   If the system has too many open files, TooManyFilesInSystem
> +#   If @filename is too long, NameTooLong
> +#   If the system doesn't have enough space, NoSpace
> +#   If the file-system to write the file to is read-only, ReadOnlyFileSystem
> +#   If opening @filename fails for an uknown reason, OpenFileFailed
> +#   If @filename becames too big, FileTooBig
> +#   If an I/O happens while writing @filename, IOError

If an I/O _error_ happens ...

Note: you expect this to be duplicated for other commands writing &
creating files, i.e. commands that will have all of the above error
conditions?

> +#
> +##
> +{ 'command': 'screendump', 'data': {'filename': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db980fa..2a3e6d5 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -146,10 +146,7 @@ EQMP
>      {
>          .name       = "screendump",
>          .args_type  = "filename:F",
> -        .params     = "filename",
> -        .help       = "save screen into PPM image 'filename'",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_screen_dump,
> +        .mhandler.cmd_new = qmp_marshal_input_screendump,
>      },
>  
>  SQMP
> -- 
> 1.7.10.2.565.gbd578b5
> 
> 

Re: [Qemu-devel] [PATCH qmp-next 00/14]: qapi: convert screendump

[Alon Levy]

On Fri, May 25, 2012 at 04:41:05PM -0300, Luiz Capitulino wrote:
> Converting the screendump command is simple and shouldn't take more than
> or or two patches, the complicated part is to report all errors correctly.
> 
> I hope I didn't go too far there, but at least this series does the right
> thing (or is very near to).
> 

Reviewed-by: Alon Levy <alevy@redhat.com>

>  console.c        |   7 ++--
>  console.h        |   5 +--
>  cutils.c         | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   5 ++-
>  hmp.c            |   9 +++++
>  hmp.h            |   1 +
>  hw/blizzard.c    |   4 +--
>  hw/g364fb.c      |  51 ++++++++++++++++++++--------
>  hw/omap_lcdc.c   |  60 +++++++++++++++++++++++----------
>  hw/qxl.c         |   7 ++--
>  hw/tcx.c         |  96 ++++++++++++++++++++++++++++++++++++++++------------
>  hw/vga.c         |  38 +++++++++++++--------
>  hw/vga_int.h     |   3 +-
>  hw/vmware_vga.c  |   7 ++--
>  monitor.c        |   6 ----
>  qapi-schema.json |  24 +++++++++++++
>  qemu-common.h    |   7 ++++
>  qerror.c         |  28 ++++++++++++++--
>  qerror.h         |  22 ++++++++++--
>  qmp-commands.hx  |   5 +--
>  20 files changed, 390 insertions(+), 95 deletions(-)

Re: [Qemu-devel] [PATCH 14/14] qapi: convert screendump

[Luiz Capitulino]

On Mon, 28 May 2012 11:56:31 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Fri, May 25, 2012 at 04:41:19PM -0300, Luiz Capitulino wrote:
> 
> One small note below.
> 
> > Also activates error reporting from devices.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  console.c        |  7 ++++---
> >  console.h        |  1 -
> >  hmp-commands.hx  |  5 ++---
> >  hmp.c            |  9 +++++++++
> >  hmp.h            |  1 +
> >  monitor.c        |  6 ------
> >  qapi-schema.json | 24 ++++++++++++++++++++++++
> >  qmp-commands.hx  |  5 +----
> >  8 files changed, 41 insertions(+), 17 deletions(-)
> > 
> > diff --git a/console.c b/console.c
> > index 4669e62..2bbf104 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -24,6 +24,7 @@
> >  #include "qemu-common.h"
> >  #include "console.h"
> >  #include "qemu-timer.h"
> > +#include "qmp-commands.h"
> >  
> >  //#define DEBUG_CONSOLE
> >  #define DEFAULT_BACKSCROLL 512
> > @@ -173,7 +174,7 @@ void vga_hw_invalidate(void)
> >          active_console->hw_invalidate(active_console->hw);
> >  }
> >  
> > -void vga_hw_screen_dump(const char *filename)
> > +void qmp_screendump(const char *filename, Error **errp)
> >  {
> >      TextConsole *previous_active_console;
> >      bool cswitch;
> > @@ -187,9 +188,9 @@ void vga_hw_screen_dump(const char *filename)
> >          console_select(0);
> >      }
> >      if (consoles[0] && consoles[0]->hw_screen_dump) {
> > -        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, NULL);
> > +        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, errp);
> >      } else {
> > -        error_report("screen dump not implemented");
> > +        error_set(errp, QERR_NOT_SUPPORTED);
> >      }
> >  
> >      if (cswitch) {
> > diff --git a/console.h b/console.h
> > index 9caeb43..d72c546 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -356,7 +356,6 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >  
> >  void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> > -void vga_hw_screen_dump(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> >  
> >  int is_graphic_console(void);
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 18cb415..74f61bc 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -193,9 +193,8 @@ ETEXI
> >          .name       = "screendump",
> >          .args_type  = "filename:F",
> >          .params     = "filename",
> > -        .help       = "save screen into PPM image 'filename'",
> > -        .user_print = monitor_user_noop,
> > -        .mhandler.cmd_new = do_screen_dump,
> > +        .help       = "save screen into PPM image 'filepath'",
> > +        .mhandler.cmd = hmp_screen_dump,
> >      },
> >  
> >  STEXI
> > diff --git a/hmp.c b/hmp.c
> > index bb0952e..7e8c0ab 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -947,3 +947,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
> >      qmp_device_del(id, &err);
> >      hmp_handle_error(mon, &err);
> >  }
> > +
> > +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_handle_error(mon, &err);
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 443b812..fa75ec0 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -61,5 +61,6 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
> >  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate(Monitor *mon, const QDict *qdict);
> >  void hmp_device_del(Monitor *mon, const QDict *qdict);
> > +void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> >  
> >  #endif
> > diff --git a/monitor.c b/monitor.c
> > index 12a6fe2..1160af7 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -897,12 +897,6 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
> >      return -1;
> >  }
> >  
> > -static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > -{
> > -    vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
> > -    return 0;
> > -}
> > -
> >  static void do_logfile(Monitor *mon, const QDict *qdict)
> >  {
> >      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 2ca7195..43d4acd 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1755,3 +1755,27 @@
> >  # Since: 0.14.0
> >  ##
> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> > +
> > +##
> > +# @screendump:
> > +#
> > +# Write a PPM of the VGA screen to a file.
> > +#
> > +# @filename: the path of a new PPM file to store the image
> > +#
> > +# Since: 0.14.0
> > +#
> > +# Returns: Nothing on success
> > +#   If the underlying device doesn't support screen dump, NotSupported
> > +#   If access to the file is not allowed, InvalidAccess
> > +#   If QEMU has too many open files, TooManyFilesByProcess
> > +#   If the system has too many open files, TooManyFilesInSystem
> > +#   If @filename is too long, NameTooLong
> > +#   If the system doesn't have enough space, NoSpace
> > +#   If the file-system to write the file to is read-only, ReadOnlyFileSystem
> > +#   If opening @filename fails for an uknown reason, OpenFileFailed
> > +#   If @filename becames too big, FileTooBig
> > +#   If an I/O happens while writing @filename, IOError
> 
> If an I/O _error_ happens ...

I'll fix it.

> Note: you expect this to be duplicated for other commands writing &
> creating files, i.e. commands that will have all of the above error
> conditions?

Yes. There could be small differences, for example, if O_EXCL is used. But
I expect most errors to be the same, yes.

> 
> > +#
> > +##
> > +{ 'command': 'screendump', 'data': {'filename': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index db980fa..2a3e6d5 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -146,10 +146,7 @@ EQMP
> >      {
> >          .name       = "screendump",
> >          .args_type  = "filename:F",
> > -        .params     = "filename",
> > -        .help       = "save screen into PPM image 'filename'",
> > -        .user_print = monitor_user_noop,
> > -        .mhandler.cmd_new = do_screen_dump,
> > +        .mhandler.cmd_new = qmp_marshal_input_screendump,
> >      },
> >  
> >  SQMP
> > -- 
> > 1.7.10.2.565.gbd578b5
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 02/14] qerror: add new errors

[Kevin Wolf]

Am 31.05.2012 18:08, schrieb Luiz Capitulino:
> On Thu, 31 May 2012 17:49:42 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 31/05/2012 17:44, Luiz Capitulino ha scritto:
>>>> One is "do not shoehorn errors into errno values".  So for QOM invalid values we
>>>> have PropertyValueBad, not a generic InvalidArgument value.  We convert everything
>>>> to Error rather than returning negative errno values and then returning generic
>>>> error codes, because those would be ugly and non-descriptive.  I agree with that.
>>>>
>>>> The other is "when errors come straight from the OS, _do_ use errno values".
>>>> This is for OpenFileFailed, for the new socket errors and so on.  This is what
>>>> I am proposing.
>>>>
>>>> These two rules together match what most other programs do.
>>>>
>>>>     $ echo | sed p > /dev/full
>>>>     sed: couldn't flush stdout: No space left on device
>>>>          ^^^^^^^^^^^^^^                                 error type
>>>>                         ^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^ arguments
>>>>
>>>> That would become, in JSON:
>>>>
>>>>     { 'error': 'FlushFailed',
>>>>       'file': 'stdout',
>>>>       'os_error': 'enospc' }
>>>
>>> Actually, I did propose something similar in the past but Anthony objected.
>>
>> Looks like in the meanwhile we moved closer to this mechanism
>> (OpenFileFailed, Sock*, etc.), except we have no clear way to identify
>> _what_ error actually happened rather than _where_.
> 
> In fact, it's the other way around. OpenFileFailed, for example, is an old
> error. We thought it would be ok to have it that way (also because of shallow
> QMP conversion, as it would take ages to convert all possible errors). But today
> it's clear it's bad and we're trying to move away from it.

It's not all that clear for me. Or actually, it is rather clear that
it's basically the right thing, but some additional information is required.

> The socket ones repeat this, but it's probably because people usually go
> for the generic error first because having detailed errors with qerror is
> cumbersome. I have some ideas to improve it that could _mitigate_ that problem,
> like having a schema file qapi-errors.json:
> 
>  { 'error': 'InvalidParameter', 'data': { 'name': 'str' } }

Errors that are as simple as that are useless if they are all you get.
Even for InvalidParameter this is true when you have more than a flat
arguments dict.

Maybe what is required in order to fully describe an error is a whole
chain of error objects that describe which error caused which other
action to fail:

{ 'error': 'TransactionCommandFailed', 'data': {
  'command': 'blockdev-snapshot-sync',
  'cause': {
      'error': 'FileOpenFailed', 'data' : {
        'filename': '/tmp/foo.img',
        'cause': {
          'error': 'PermissionDenied', 'data': {}
        }
      }
   }
}

Not sure if that would be easy to process for a QMP client, though...

Kevin