[Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Marc-André Lureau]

Hi,

The current screendump command can only save to disk very large PPM files.

The following patches add support for screendump in a UNIX socket, following the syntax used for migration URI: "unix:/path/to/socket". 

The last 3 patches add support for dumping in PNG format. This can reduce the size of the image by a great factor (x10 is not unusual), and is also a more convenient format than PPM. Currently, it dumps in PNG by checking if the path ends with ".png", we may want to have a seperate option for that instead, or use this syntax only if the path begins with "unix:" (or any foo:) for example.

Next, I would like to add support for a scaling factor too (a typical use case is to show a small thumbnail of the desktop). Specifying only the requested "width" or "height" should be supported. Should we rely on pixman to do this work?

Finally, those functions should not be blocking. With a bit of guidance, I could work on a follow-up patch adding support for it, but for now there are related work still in discussion (screendump-async etc.) and I thought it was best to avoid.

regards

Marc-André Lureau (7):
  ppm_save: use QEMUFile
  Allow a qemu_fopen_socket() to be opened for writing
  Close socket when closing QEMUFile
  Allow saving screendump to a UNIX socket
  configure: split PNG support from vnc_png feature
  Isolate color conversion from PPM handling
  Add PNG screendump

 Makefile.target  |    2 +-
 configure        |   42 +++++++++---
 hw/vga.c         |  197 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 migration-tcp.c  |   11 ++--
 migration-unix.c |   11 ++--
 qemu-file.h      |    3 +-
 savevm.c         |   37 +++++++++-
 7 files changed, 260 insertions(+), 43 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 1/7] ppm_save: use QEMUFile

[Marc-André Lureau]

---
 hw/vga.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 5994f43..24af4a1 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2364,19 +2364,20 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 
 int ppm_save(const char *filename, struct DisplaySurface *ds)
 {
-    FILE *f;
+    QEMUFile *f;
     uint8_t *d, *d1;
     uint32_t v;
     int y, x;
     uint8_t r, g, b;
-    int ret;
     char *linebuf, *pbuf;
+    gchar *header;
+
+    f = qemu_fopen(filename, "wb");
+    g_return_val_if_fail(f != NULL, -1);
+
+    header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
+    qemu_put_buffer(f, (uint8_t*)header, strlen(header));
 
-    f = fopen(filename, "wb");
-    if (!f)
-        return -1;
-    fprintf(f, "P6\n%d %d\n%d\n",
-            ds->width, ds->height, 255);
     linebuf = g_malloc(ds->width * 3);
     d1 = ds->data;
     for(y = 0; y < ds->height; y++) {
@@ -2397,11 +2398,12 @@ 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_put_buffer(f, (uint8_t*)linebuf, pbuf - linebuf);
     }
+
     g_free(linebuf);
-    fclose(f);
+    g_free(header);
+    qemu_fclose(f);
     return 0;
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 2/7] Allow a qemu_fopen_socket() to be opened for writing

[Marc-André Lureau]

---
 migration-tcp.c  |    2 +-
 migration-unix.c |    2 +-
 qemu-file.h      |    2 +-
 savevm.c         |   36 ++++++++++++++++++++++++++++++++----
 4 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..f567898 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -140,7 +140,7 @@ static void tcp_accept_incoming_migration(void *opaque)
         goto out2;
     }
 
-    f = qemu_fopen_socket(c);
+    f = qemu_fopen_socket(c, "r");
     if (f == NULL) {
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
diff --git a/migration-unix.c b/migration-unix.c
index 169de88..3928f4c 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -137,7 +137,7 @@ static void unix_accept_incoming_migration(void *opaque)
         goto out2;
     }
 
-    f = qemu_fopen_socket(c);
+    f = qemu_fopen_socket(c, "r");
     if (f == NULL) {
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
diff --git a/qemu-file.h b/qemu-file.h
index 31b83f6..efd6b1c 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -67,7 +67,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
                          QEMUFileGetRateLimit *get_rate_limit);
 QEMUFile *qemu_fopen(const char *filename, const char *mode);
 QEMUFile *qemu_fdopen(int fd, const char *mode);
-QEMUFile *qemu_fopen_socket(int fd);
+QEMUFile *qemu_fopen_socket(int fd, const char *mode);
 QEMUFile *qemu_popen(FILE *popen_file, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_stdio_fd(QEMUFile *f);
diff --git a/savevm.c b/savevm.c
index 80be1ff..4171ebf 100644
--- a/savevm.c
+++ b/savevm.c
@@ -205,6 +205,21 @@ static int socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
     return len;
 }
 
+static int socket_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
+{
+    QEMUFileSocket *s = opaque;
+    ssize_t len;
+
+    do {
+        len = write(s->fd, buf, size);
+    } while (len == -1 && socket_error() == EINTR);
+
+    if (len == -1)
+        len = -socket_error();
+
+    return len;
+}
+
 static int socket_close(void *opaque)
 {
     QEMUFileSocket *s = opaque;
@@ -316,7 +331,7 @@ QEMUFile *qemu_fdopen(int fd, const char *mode)
     if (!s->stdio_file)
         goto fail;
 
-    if(mode[0] == 'r') {
+    if (mode[0] == 'r') {
         s->file = qemu_fopen_ops(s, NULL, stdio_get_buffer, stdio_fclose, 
 				 NULL, NULL, NULL);
     } else {
@@ -330,13 +345,26 @@ fail:
     return NULL;
 }
 
-QEMUFile *qemu_fopen_socket(int fd)
+QEMUFile *qemu_fopen_socket(int fd, const char *mode)
 {
     QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
 
+    if (mode == NULL ||
+	(mode[0] != 'r' && mode[0] != 'w') ||
+	mode[1] != 'b' || mode[2] != 0) {
+        fprintf(stderr, "qemu_fopen_socket: Argument validity check failed\n");
+        return NULL;
+    }
+
     s->fd = fd;
-    s->file = qemu_fopen_ops(s, NULL, socket_get_buffer, socket_close, 
-			     NULL, NULL, NULL);
+    if (mode[0] == 'r') {
+        s->file = qemu_fopen_ops(s, NULL, socket_get_buffer, socket_close,
+                                 NULL, NULL, NULL);
+    } else {
+        s->file = qemu_fopen_ops(s, socket_put_buffer, NULL, socket_close,
+                                 NULL, NULL, NULL);
+    }
+
     return s->file;
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 3/7] Close socket when closing QEMUFile

[Marc-André Lureau]

---
 migration-tcp.c  |    9 +++++----
 migration-unix.c |    9 +++++----
 savevm.c         |    1 +
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index f567898..056867c 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -137,7 +137,7 @@ static void tcp_accept_incoming_migration(void *opaque)
 
     if (c == -1) {
         fprintf(stderr, "could not accept migration connection\n");
-        goto out2;
+        goto out;
     }
 
     f = qemu_fopen_socket(c, "r");
@@ -145,12 +145,13 @@ static void tcp_accept_incoming_migration(void *opaque)
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
     }
-
+    c = -1;
     process_incoming_migration(f);
     qemu_fclose(f);
+
 out:
-    close(c);
-out2:
+    if (c != -1)
+        close(c);
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     close(s);
 }
diff --git a/migration-unix.c b/migration-unix.c
index 3928f4c..0a3a076 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -134,7 +134,7 @@ static void unix_accept_incoming_migration(void *opaque)
 
     if (c == -1) {
         fprintf(stderr, "could not accept migration connection\n");
-        goto out2;
+        goto out;
     }
 
     f = qemu_fopen_socket(c, "r");
@@ -142,12 +142,13 @@ static void unix_accept_incoming_migration(void *opaque)
         fprintf(stderr, "could not qemu_fopen socket\n");
         goto out;
     }
-
+    c = -1;
     process_incoming_migration(f);
     qemu_fclose(f);
+
 out:
-    close(c);
-out2:
+    if (c != -1)
+        close(c);
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     close(s);
 }
diff --git a/savevm.c b/savevm.c
index 4171ebf..b6a690d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -223,6 +223,7 @@ static int socket_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int
 static int socket_close(void *opaque)
 {
     QEMUFileSocket *s = opaque;
+    close(s->fd);
     g_free(s);
     return 0;
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 4/7] Allow saving screendump to a UNIX socket

[Marc-André Lureau]

By using the 'unix:' prefix notation, similar to the migration uri, we
can now dump to a UNIX socket. IO is still sync
---
 hw/vga.c    |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-file.h |    1 +
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 24af4a1..bb5df70 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -30,6 +30,7 @@
 #include "pixel_ops.h"
 #include "qemu-timer.h"
 #include "xen.h"
+#include "qemu_socket.h"
 
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
@@ -37,6 +38,14 @@
 
 //#define DEBUG_BOCHS_VBE
 
+#ifdef DEBUG_VGA
+#define DPRINTF(fmt, ...) \
+    do { printf("vga: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+    do { } while (0)
+#endif
+
 /*
  * Video Graphics Array (VGA)
  *
@@ -2362,7 +2371,58 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 /********************************************************/
 /* vga screen dump */
 
-int ppm_save(const char *filename, struct DisplaySurface *ds)
+#if !defined(WIN32)
+static QEMUFile *qemu_fopen_unix(const char *path, const char *mode)
+{
+    struct sockaddr_un addr;
+    int ret, fd;
+
+    addr.sun_family = AF_UNIX;
+    snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
+
+    fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
+    if (fd == -1) {
+        DPRINTF("Unable to open socket: %s\n", strerror(errno));
+        return NULL;
+    }
+
+    do {
+        ret = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
+        if (ret == -1) {
+            ret = -errno;
+        }
+    } while (ret == -EINTR);
+
+    if (ret < 0) {
+        DPRINTF("connect failed: %s\n": strerror(errno));
+        return NULL;
+    }
+
+    return qemu_fopen_socket(fd, mode);
+}
+#endif
+
+QEMUFile *qemu_fopen_uri(const char *uri, const char *mode)
+{
+    QEMUFile *f = NULL;
+    const char *p;
+
+    g_return_val_if_fail(uri != NULL, NULL);
+    g_return_val_if_fail(mode != NULL, NULL);
+
+#if !defined(WIN32)
+    if (strstart(uri, "unix:", &p)) {
+        f = qemu_fopen_unix(p, mode);
+    }
+#endif
+    if (f == NULL) {
+        f = qemu_fopen(uri, mode);
+    }
+
+    return f;
+}
+
+int ppm_save(const char *uri, struct DisplaySurface *ds)
 {
     QEMUFile *f;
     uint8_t *d, *d1;
@@ -2372,7 +2432,7 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
     char *linebuf, *pbuf;
     gchar *header;
 
-    f = qemu_fopen(filename, "wb");
+    f = qemu_fopen_uri(uri, "wb");
     g_return_val_if_fail(f != NULL, -1);
 
     header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
diff --git a/qemu-file.h b/qemu-file.h
index efd6b1c..0914d83 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -68,6 +68,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
 QEMUFile *qemu_fopen(const char *filename, const char *mode);
 QEMUFile *qemu_fdopen(int fd, const char *mode);
 QEMUFile *qemu_fopen_socket(int fd, const char *mode);
+QEMUFile *qemu_fopen_uri(const char *uri, const char *mode);
 QEMUFile *qemu_popen(FILE *popen_file, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_stdio_fd(QEMUFile *f);
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 5/7] configure: split PNG support from vnc_png feature

[Marc-André Lureau]

---
 Makefile.target |    2 +-
 configure       |   42 ++++++++++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 1bd25a8..e5273f2 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -215,7 +215,7 @@ obj-i386-$(CONFIG_KVM) += hyperv.o
 QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
 QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
 QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
-QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
+QEMU_CFLAGS += $(PNG_CFLAGS)
 
 # xen support
 obj-$(CONFIG_XEN) += xen-all.o xen_machine_pv.o xen_domainbuild.o xen-mapcache.o
diff --git a/configure b/configure
index 39d2b54..3abdf3f 100755
--- a/configure
+++ b/configure
@@ -123,6 +123,7 @@ curses=""
 docs=""
 fdt=""
 nptl=""
+png=""
 sdl=""
 virtfs=""
 vnc="yes"
@@ -602,6 +603,10 @@ for opt do
     # configure to be used by RPM and similar macros that set
     # lots of directory switches by default.
   ;;
+  --disable-png) png="no"
+  ;;
+  --enable-png) png="yes"
+  ;;
   --disable-sdl) sdl="no"
   ;;
   --enable-sdl) sdl="yes"
@@ -1043,6 +1048,8 @@ echo "  --disable-vnc-png        disable PNG compression for VNC server (default
 echo "  --enable-vnc-png         enable PNG compression for VNC server"
 echo "  --disable-vnc-thread     disable threaded VNC server"
 echo "  --enable-vnc-thread      enable threaded VNC server"
+echo "  --disable-png            disable PNG compression (default)"
+echo "  --enable-png             enable PNG compression"
 echo "  --disable-curses         disable curses output"
 echo "  --enable-curses          enable curses output"
 echo "  --disable-curl           disable curl connectivity"
@@ -1648,8 +1655,8 @@ EOF
 fi
 
 ##########################################
-# VNC PNG detection
-if test "$vnc" = "yes" -a "$vnc_png" != "no" ; then
+# PNG detection
+if test "$png" != "no"; then
 cat > $TMPC <<EOF
 //#include <stdio.h>
 #include <png.h>
@@ -1661,16 +1668,27 @@ int main(void) {
 }
 EOF
   if $pkg_config libpng --modversion >/dev/null 2>&1; then
-    vnc_png_cflags=`$pkg_config libpng --cflags 2> /dev/null`
-    vnc_png_libs=`$pkg_config libpng --libs 2> /dev/null`
+    png_cflags=`$pkg_config libpng --cflags 2> /dev/null`
+    png_libs=`$pkg_config libpng --libs 2> /dev/null`
+  else
+    png_cflags=""
+    png_libs="-lpng"
+  fi
+  if compile_prog "$png_cflags" "$png_libs" ; then
+    png=yes
+    libs_softmmu="$png_libs $libs_softmmu"
+    QEMU_CFLAGS="$QEMU_CFLAGS $png_cflags"
   else
-    vnc_png_cflags=""
-    vnc_png_libs="-lpng"
+    if test "$png" = "yes" ; then
+      feature_not_found "png"
+    fi
+    png=no
   fi
-  if compile_prog "$vnc_png_cflags" "$vnc_png_libs" ; then
+fi
+
+if test "$vnc" = "yes" -a "$vnc_png" != "no"; then
+  if test "$png" = "yes" ; then
     vnc_png=yes
-    libs_softmmu="$vnc_png_libs $libs_softmmu"
-    QEMU_CFLAGS="$QEMU_CFLAGS $vnc_png_cflags"
   else
     if test "$vnc_png" = "yes" ; then
       feature_not_found "vnc-png"
@@ -2879,6 +2897,7 @@ if test "$darwin" = "yes" ; then
     echo "Cocoa support     $cocoa"
 fi
 echo "SDL support       $sdl"
+echo "PNG support       $png"
 echo "curses support    $curses"
 echo "curl support      $curl"
 echo "mingw32 support   $mingw32"
@@ -3067,7 +3086,10 @@ if test "$vnc_jpeg" = "yes" ; then
 fi
 if test "$vnc_png" = "yes" ; then
   echo "CONFIG_VNC_PNG=y" >> $config_host_mak
-  echo "VNC_PNG_CFLAGS=$vnc_png_cflags" >> $config_host_mak
+fi
+if test "$png" = "yes" ; then
+  echo "PNG_CFLAGS=$png_cflags" >> $config_host_mak
+  echo "CONFIG_PNG=y" >> $config_host_mak
 fi
 if test "$vnc_thread" = "yes" ; then
   echo "CONFIG_VNC_THREAD=y" >> $config_host_mak
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 6/7] Isolate color conversion from PPM handling

[Marc-André Lureau]

---
 hw/vga.c |   62 +++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index bb5df70..9f7ca89 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2422,32 +2422,37 @@ QEMUFile *qemu_fopen_uri(const char *uri, const char *mode)
     return f;
 }
 
-int ppm_save(const char *uri, struct DisplaySurface *ds)
+static int ppm_line_handler(void *opaque, uint8_t *line, size_t length)
 {
-    QEMUFile *f;
+    QEMUFile *f = opaque;
+
+    qemu_put_buffer(f, line, length);
+
+    return length;
+}
+
+static int screen_dump_lines(struct DisplaySurface *ds,
+    int (*line_handler) (void *opaque, uint8_t *data, size_t length),
+    void *opaque)
+{
+    char *linebuf, *pbuf;
     uint8_t *d, *d1;
     uint32_t v;
     int y, x;
     uint8_t r, g, b;
-    char *linebuf, *pbuf;
-    gchar *header;
-
-    f = qemu_fopen_uri(uri, "wb");
-    g_return_val_if_fail(f != NULL, -1);
-
-    header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
-    qemu_put_buffer(f, (uint8_t*)header, strlen(header));
+    int ret = -1;
 
     linebuf = g_malloc(ds->width * 3);
     d1 = ds->data;
-    for(y = 0; y < ds->height; y++) {
+    for (y = 0; y < ds->height; y++) {
         d = d1;
         pbuf = linebuf;
-        for(x = 0; x < ds->width; x++) {
-            if (ds->pf.bits_per_pixel == 32)
+        for (x = 0; x < ds->width; x++) {
+            if (ds->pf.bits_per_pixel == 32) {
                 v = *(uint32_t *)d;
-            else
+            } else {
                 v = (uint32_t) (*(uint16_t *)d);
+            }
             /* Limited to 8 or fewer bits per channel: */
             r = ((v >> ds->pf.rshift) & ds->pf.rmax) << (8 - ds->pf.rbits);
             g = ((v >> ds->pf.gshift) & ds->pf.gmax) << (8 - ds->pf.gbits);
@@ -2458,13 +2463,36 @@ int ppm_save(const char *uri, struct DisplaySurface *ds)
             d += ds->pf.bytes_per_pixel;
         }
         d1 += ds->linesize;
-        qemu_put_buffer(f, (uint8_t*)linebuf, pbuf - linebuf);
+
+        if (line_handler(opaque, (uint8_t*)linebuf, pbuf - linebuf) < 0)
+            goto out;
     }
 
+    ret = 0;
+out:
     g_free(linebuf);
-    g_free(header);
+    return ret;
+}
+
+int ppm_save(const char *uri, struct DisplaySurface *ds)
+{
+    QEMUFile *f;
+    int ret = -1;
+
+    f = qemu_fopen_uri(uri, "wb");
+    g_return_val_if_fail(f != NULL, -1);
+
+    {
+        gchar *header;
+        header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
+        qemu_put_buffer(f, (uint8_t*)header, strlen(header));
+        screen_dump_lines(ds, ppm_line_handler, f);
+        g_free(header);
+    }
+
+    ret = 0;
     qemu_fclose(f);
-    return 0;
+    return ret;
 }
 
 /* save the vga display in a PPM image even if no display is
-- 
1.7.7.6

[Qemu-devel] [PATCH/RFC 7/7] Add PNG screendump

[Marc-André Lureau]

Dump an image in PNG format if the URI ends with ".png" and PNG
support is enabled.
---
 hw/vga.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 9f7ca89..0210cde 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -32,6 +32,13 @@
 #include "xen.h"
 #include "qemu_socket.h"
 
+#ifdef CONFIG_PNG
+/* The following define is needed by pngconf.h. Otherwise it won't compile,
+   because setjmp.h was already included by qemu-common.h. */
+#define PNG_SKIP_SETJMP_CHECK
+#include <png.h>
+#endif
+
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
 //#define DEBUG_VGA_REG
@@ -2422,6 +2429,39 @@ QEMUFile *qemu_fopen_uri(const char *uri, const char *mode)
     return f;
 }
 
+#ifdef CONFIG_PNG
+static void *vga_png_malloc(png_structp png_ptr, png_size_t size)
+{
+    return g_malloc(size);
+}
+
+static void vga_png_free(png_structp png_ptr, png_voidp ptr)
+{
+    g_free(ptr);
+}
+
+static void vga_png_write_data(png_structp png_ptr, png_bytep data,
+                               png_size_t length)
+{
+    QEMUFile *f = png_get_io_ptr(png_ptr);
+
+    qemu_put_buffer(f, (uint8_t*)data, length);
+}
+
+static void vga_png_flush_data(png_structp png_ptr)
+{
+}
+
+static int png_line_handler(void *opaque, uint8_t *line, size_t length)
+{
+    png_structp png_ptr = opaque;
+
+    png_write_row(png_ptr, line);
+
+    return length;
+}
+#endif
+
 static int ppm_line_handler(void *opaque, uint8_t *line, size_t length)
 {
     QEMUFile *f = opaque;
@@ -2482,7 +2522,39 @@ int ppm_save(const char *uri, struct DisplaySurface *ds)
     f = qemu_fopen_uri(uri, "wb");
     g_return_val_if_fail(f != NULL, -1);
 
-    {
+    if (g_str_has_suffix(uri, ".png")) {
+#ifdef CONFIG_PNG
+        png_structp png_ptr;
+        png_infop info_ptr;
+
+        png_ptr = png_create_write_struct_2(PNG_LIBPNG_VER_STRING,
+                                            NULL, NULL, NULL,
+                                            NULL, vga_png_malloc,
+                                            vga_png_free);
+        if (png_ptr == NULL) {
+            goto out;
+        }
+
+        info_ptr = png_create_info_struct(png_ptr);
+        if (info_ptr == NULL) {
+            png_destroy_write_struct(&png_ptr, NULL);
+            goto out;
+        }
+
+        png_set_write_fn(png_ptr, (void *)f,
+                         vga_png_write_data, vga_png_flush_data);
+        png_set_IHDR(png_ptr, info_ptr, ds->width, ds->height,
+                     8, PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
+                     PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
+        png_write_info(png_ptr, info_ptr);
+        screen_dump_lines(ds, png_line_handler, png_ptr);
+        png_write_end(png_ptr, NULL);
+        png_destroy_write_struct(&png_ptr, &info_ptr);
+#else
+        DPRINTF("PNG support is disabled");
+        goto out;
+#endif
+    } else {
         gchar *header;
         header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
         qemu_put_buffer(f, (uint8_t*)header, strlen(header));
@@ -2491,6 +2563,7 @@ int ppm_save(const char *uri, struct DisplaySurface *ds)
     }
 
     ret = 0;
+out:
     qemu_fclose(f);
     return ret;
 }
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

On 03/12/2012 07:11 AM, Marc-André Lureau wrote:
> Hi,
> 
> The current screendump command can only save to disk very large PPM files.
> 
> The following patches add support for screendump in a UNIX socket, following the syntax used for migration URI: "unix:/path/to/socket". 

Libvirt would also benefit from screendump to fd:name (where name refers
to an fd passed via getfd).

> 
> The last 3 patches add support for dumping in PNG format. This can reduce the size of the image by a great factor (x10 is not unusual), and is also a more convenient format than PPM. Currently, it dumps in PNG by checking if the path ends with ".png", we may want to have a seperate option for that instead,

You will need a separate option for this, if you add fd:name parsing,
since name won't necessarily have a .png suffix (I guess libvirt could
use getfd to name an fd with a .png suffix, but that seems weird).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH/RFC 7/7] Add PNG screendump

[Daniel P. Berrange]

On Mon, Mar 12, 2012 at 02:11:33PM +0100, Marc-André Lureau wrote:
> Dump an image in PNG format if the URI ends with ".png" and PNG
> support is enabled.
> ---
>  hw/vga.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 74 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index 9f7ca89..0210cde 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -32,6 +32,13 @@
>  #include "xen.h"
>  #include "qemu_socket.h"
>  
> +#ifdef CONFIG_PNG
> +/* The following define is needed by pngconf.h. Otherwise it won't compile,
> +   because setjmp.h was already included by qemu-common.h. */
> +#define PNG_SKIP_SETJMP_CHECK
> +#include <png.h>
> +#endif

[snip]

I'd be inclined to just switch over to use gdk-pixbuf for saving
images. It copes with a wide array of formats, and is a natrual
fit now that we're using GLib.  NB, gdk-pixbuf is distributed and
built separate from GDK/GTK, so this wouldn't create a hard dep
on GTK - you could still build with just the gdk-pixbuf support.

While allowing gdk-pixbuf to guess format based off the file
name is fine, I'd suggest we should add an explicit format argument
to the 'screendump' command. If omitted, the format would be
guessed from filename if possible.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH/RFC 4/7] Allow saving screendump to a UNIX socket

[Daniel P. Berrange]

On Mon, Mar 12, 2012 at 02:11:30PM +0100, Marc-André Lureau wrote:
> By using the 'unix:' prefix notation, similar to the migration uri, we
> can now dump to a UNIX socket. IO is still sync
> ---
>  hw/vga.c    |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-file.h |    1 +
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index 24af4a1..bb5df70 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -30,6 +30,7 @@
>  #include "pixel_ops.h"
>  #include "qemu-timer.h"
>  #include "xen.h"
> +#include "qemu_socket.h"
>  
>  //#define DEBUG_VGA
>  //#define DEBUG_VGA_MEM
> @@ -37,6 +38,14 @@
>  
>  //#define DEBUG_BOCHS_VBE
>  
> +#ifdef DEBUG_VGA
> +#define DPRINTF(fmt, ...) \
> +    do { printf("vga: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
>  /*
>   * Video Graphics Array (VGA)
>   *
> @@ -2362,7 +2371,58 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
>  /********************************************************/
>  /* vga screen dump */
>  
> -int ppm_save(const char *filename, struct DisplaySurface *ds)
> +#if !defined(WIN32)
> +static QEMUFile *qemu_fopen_unix(const char *path, const char *mode)
> +{
> +    struct sockaddr_un addr;
> +    int ret, fd;
> +
> +    addr.sun_family = AF_UNIX;
> +    snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
> +
> +    fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> +    if (fd == -1) {
> +        DPRINTF("Unable to open socket: %s\n", strerror(errno));
> +        return NULL;
> +    }
> +
> +    do {
> +        ret = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
> +        if (ret == -1) {
> +            ret = -errno;
> +        }
> +    } while (ret == -EINTR);
> +
> +    if (ret < 0) {
> +        DPRINTF("connect failed: %s\n": strerror(errno));
> +        return NULL;
> +    }
> +
> +    return qemu_fopen_socket(fd, mode);
> +}
> +#endif
> +
> +QEMUFile *qemu_fopen_uri(const char *uri, const char *mode)
> +{
> +    QEMUFile *f = NULL;
> +    const char *p;
> +
> +    g_return_val_if_fail(uri != NULL, NULL);
> +    g_return_val_if_fail(mode != NULL, NULL);
> +
> +#if !defined(WIN32)
> +    if (strstart(uri, "unix:", &p)) {
> +        f = qemu_fopen_unix(p, mode);
> +    }
> +#endif
> +    if (f == NULL) {
> +        f = qemu_fopen(uri, mode);
> +    }
> +
> +    return f;
> +}
> +
> +int ppm_save(const char *uri, struct DisplaySurface *ds)
>  {
>      QEMUFile *f;
>      uint8_t *d, *d1;
> @@ -2372,7 +2432,7 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>      char *linebuf, *pbuf;
>      gchar *header;
>  
> -    f = qemu_fopen(filename, "wb");
> +    f = qemu_fopen_uri(uri, "wb");
>      g_return_val_if_fail(f != NULL, -1);
>  
>      header = g_strdup_printf("P6\n%d %d\n%d\n", ds->width, ds->height, 255);
> diff --git a/qemu-file.h b/qemu-file.h
> index efd6b1c..0914d83 100644
> --- a/qemu-file.h
> +++ b/qemu-file.h
> @@ -68,6 +68,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
>  QEMUFile *qemu_fopen(const char *filename, const char *mode);
>  QEMUFile *qemu_fdopen(int fd, const char *mode);
>  QEMUFile *qemu_fopen_socket(int fd, const char *mode);
> +QEMUFile *qemu_fopen_uri(const char *uri, const char *mode);
>  QEMUFile *qemu_popen(FILE *popen_file, const char *mode);
>  QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
>  int qemu_stdio_fd(QEMUFile *f);

It would be even more broadly useful if the screendump command was
extended to allow a file descriptor to be just passed across to
QEMU, avoiding the need to interact with the filesystem namespace
at all.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Daniel P. Berrange]

On Mon, Mar 12, 2012 at 02:11:26PM +0100, Marc-André Lureau wrote:
> Hi,
> 
> The current screendump command can only save to disk very large PPM files.
> 
> The following patches add support for screendump in a UNIX socket,
> following the syntax used for migration URI: "unix:/path/to/socket".

To facilitate interaction with security frameworks like SELinbux and
AppArmour, it is even better for the mgmt app to just be able to pass a
pre-opened FD across the monitor, rather than using UNIX sockets.


> The last 3 patches add support for dumping in PNG format. This can
> reduce the size of the image by a great factor (x10 is not unusual),
> and is also a more convenient format than PPM. Currently, it dumps
> in PNG by checking if the path ends with ".png", we may want to
> have a seperate option for that instead, or use this syntax only
> if the path begins with "unix:" (or any foo:) for example.

We can let it guess format by default, but we should provide a way
to force the format via an optional arg.

> Next, I would like to add support for a scaling factor too (a typical
> use case is to show a small thumbnail of the desktop). Specifying
> only the requested "width" or "height" should be supported. Should
> we rely on pixman to do this work?

My suggestion would be to use Gdk-Pixbuf, since that also gives you support
for saving to arbitrary file formats, as well as scaling, etc.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 02:11:26PM +0100, Marc-André Lureau wrote:
> Next, I would like to add support for a scaling factor too (a typical use case is to show a small thumbnail of the desktop). Specifying only the requested "width" or "height" should be supported. Should we rely on pixman to do this work?

This is taking it a bit far.  There are many different use cases for
screenshots and coding the thumbnail use case specifically into QEMU
doesn't seem like the way to go.

I think ppm for lossless and png for small file size are good options to
have.  Beyond that it's up to the caller to deal with the screenshot,
which might be scaling, detecting diffs, encoding in a fancier format,
etc.

Stefan

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Anthony Liguori]

On 03/12/2012 08:11 AM, Marc-André Lureau wrote:
> Hi,
>
> The current screendump command can only save to disk very large PPM files.
>
> The following patches add support for screendump in a UNIX socket, following the syntax used for migration URI: "unix:/path/to/socket".

Why not just return the screendump through QMP?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Marc-André Lureau]

On Mon, Mar 12, 2012 at 7:53 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Why not just return the screendump through QMP?

in base64, base85?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Anthony Liguori]

On 03/12/2012 01:56 PM, Marc-André Lureau wrote:
> On Mon, Mar 12, 2012 at 7:53 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> Why not just return the screendump through QMP?
>
> in base64, base85?

I think Jan had patches for base64 blobs but I think it got tangled up in 
VMState related controversy.

Jan, am I correct?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Marc-André Lureau]

On Mon, Mar 12, 2012 at 7:06 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> I think ppm for lossless and png for small file size are good options to
> have.  Beyond that it's up to the caller to deal with the screenshot,
> which might be scaling, detecting diffs, encoding in a fancier format,
> etc.

Daniel Berrange suggested we use gdk-pixbuf for saving into various
formats and scaling support. If we make it optionnal, it brings a few
more dependencies although the library itself is fairly small (140k,
but adds gobject), and we would gain optionnal support for other
formats (noteably jpeg) and scaling.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Marc-André Lureau]

Hi

On Mon, Mar 12, 2012 at 4:42 PM, Eric Blake <eblake@redhat.com> wrote:
> Libvirt would also benefit from screendump to fd:name (where name refers
> to an fd passed via getfd).

Is this all it takes? or am I missing something?

+    } else if (strstart(uri, "fd:", &p)) {
+        int fd = strtol(p, NULL, 0);
+        f = qemu_fdopen(fd, mode);


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Michael Roth]

On Mon, Mar 12, 2012 at 01:57:14PM -0500, Anthony Liguori wrote:
> On 03/12/2012 01:56 PM, Marc-André Lureau wrote:
> >On Mon, Mar 12, 2012 at 7:53 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >>Why not just return the screendump through QMP?
> >
> >in base64, base85?
> 
> I think Jan had patches for base64 blobs but I think it got tangled
> up in VMState related controversy.

Few months back, was planning on using them for qemu-ga. Looks like
they're not in yet, but FWIW qemu-ga has been using g_base64_encode() since
initial commit. I think Jan had some reservations about g_base64_decode()'s
error-handling, but that shouldn't be an issue in this particular case.

I agree on the QMP approach sounding a whole lot cleaner, especially
given that Juan recently did a some cleanups to remove non-migration
QEMUFile users:

3a230256e8418f5cc9761f36feb6952d7ca38d73

> 
> Jan, am I correct?
> 
> Regards,
> 
> Anthony Liguori
> 
> 

Re: [Qemu-devel] [PATCH/RFC 3/7] Close socket when closing QEMUFile

[Igor Mitsyanko]

On 03/12/2012 05:11 PM, Marc-André Lureau wrote:
> ---
>   migration-tcp.c  |    9 +++++----
>   migration-unix.c |    9 +++++----
>   savevm.c         |    1 +
>   3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index f567898..056867c 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -137,7 +137,7 @@ static void tcp_accept_incoming_migration(void *opaque)
>
>       if (c == -1) {
>           fprintf(stderr, "could not accept migration connection\n");
> -        goto out2;
> +        goto out;
>       }
>
>       f = qemu_fopen_socket(c, "r");
> @@ -145,12 +145,13 @@ static void tcp_accept_incoming_migration(void *opaque)
>           fprintf(stderr, "could not qemu_fopen socket\n");
>           goto out;
>       }
> -
> +    c = -1;
>       process_incoming_migration(f);
>       qemu_fclose(f);
> +
>   out:
> -    close(c);
> -out2:
> +    if (c != -1)
> +        close(c);
>       qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>       close(s);
>   }
> diff --git a/migration-unix.c b/migration-unix.c
> index 3928f4c..0a3a076 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -134,7 +134,7 @@ static void unix_accept_incoming_migration(void *opaque)
>
>       if (c == -1) {
>           fprintf(stderr, "could not accept migration connection\n");
> -        goto out2;
> +        goto out;
>       }
>
>       f = qemu_fopen_socket(c, "r");
> @@ -142,12 +142,13 @@ static void unix_accept_incoming_migration(void *opaque)
>           fprintf(stderr, "could not qemu_fopen socket\n");
>           goto out;
>       }
> -
> +    c = -1;
>       process_incoming_migration(f);
>       qemu_fclose(f);
> +
>   out:
> -    close(c);
> -out2:
> +    if (c != -1)
> +        close(c);
>       qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>       close(s);
>   }

As I understand you want to close(c) only if (f == NULL) succeeded, and 
do not want to do this in normal function path? It looks strange (I'm 
not an expert though), but if this is actually correct, than you can do 
this in "if (f == NULL)" body, without checking if (c != -1).

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com

Re: [Qemu-devel] [PATCH/RFC 4/7] Allow saving screendump to a UNIX socket

[Gerd Hoffmann]

  Hi,

> By using the 'unix:' prefix notation, similar to the migration uri, we
> can now dump to a UNIX socket. IO is still sync

>  hw/vga.c    |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

This calls for a screendump.c IMHO.

vga.c is big enougth already and screendump hasn't much to do with vga,
it is a generic service which should work with every qemu-emulated
display adapter (and it actually does, ppm_save takes a
hardware-independant displaysurface and is called from a few non-vga
places).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Jan Kiszka]

On 2012-03-12 22:22, Michael Roth wrote:
> On Mon, Mar 12, 2012 at 01:57:14PM -0500, Anthony Liguori wrote:
>> On 03/12/2012 01:56 PM, Marc-André Lureau wrote:
>>> On Mon, Mar 12, 2012 at 7:53 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>>> Why not just return the screendump through QMP?
>>>
>>> in base64, base85?
>>
>> I think Jan had patches for base64 blobs but I think it got tangled
>> up in VMState related controversy.
> 
> Few months back, was planning on using them for qemu-ga. Looks like
> they're not in yet, but FWIW qemu-ga has been using g_base64_encode() since
> initial commit. I think Jan had some reservations about g_base64_decode()'s
> error-handling, but that shouldn't be an issue in this particular case.

Yes, please don't use glib for this, just introduce a proper
error-checking implementation to QEMU. glib would require some revision
of this API to fix its deficits (still possible long-term, though).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 7:27 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> On Mon, Mar 12, 2012 at 7:06 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> I think ppm for lossless and png for small file size are good options to
>> have.  Beyond that it's up to the caller to deal with the screenshot,
>> which might be scaling, detecting diffs, encoding in a fancier format,
>> etc.
>
> Daniel Berrange suggested we use gdk-pixbuf for saving into various
> formats and scaling support. If we make it optionnal, it brings a few
> more dependencies although the library itself is fairly small (140k,
> but adds gobject), and we would gain optionnal support for other
> formats (noteably jpeg) and scaling.

Yeah it wants png, jpeg, tiff.  What I'm concerned about is that QEMU
should be doing emulation/virtualization, not processing images for an
end-user.  Its main loop is not structured to do utility tasks
unrelated to running the guest.  If implemented properly QEMU isn't a
good place to host this processing, and if done badly we block the
guest and make it unresponsive.  This is a general architectural
problem, not something specific to your proposal but this is why I'm
against putting this into QEMU.

If you send a ppm over a file descriptor, what's the fundamental
limitation which requires you to add this code to QEMU?  Perhaps
simply supporting file descriptors is the right level of support from
QEMU.

Stefan

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Marc-André Lureau]

Hi

----- Mensaje original -----
> Yeah it wants png, jpeg, tiff.  What I'm concerned about is that QEMU
> should be doing emulation/virtualization, not processing images for
> an
> end-user.  Its main loop is not structured to do utility tasks
> unrelated to running the guest.  If implemented properly QEMU isn't a
> good place to host this processing, and if done badly we block the
> guest and make it unresponsive.  This is a general architectural
> problem, not something specific to your proposal but this is why I'm
> against putting this into QEMU.
> 
> If you send a ppm over a file descriptor, what's the fundamental
> limitation which requires you to add this code to QEMU?  Perhaps
> simply supporting file descriptors is the right level of support from
> QEMU.

That sounds reasonable to me, I would even remove the PPM support, and the format conversion in this case.

It needs to be done somewhere though, and it seems reasonable to do it from the source.

What if we fork a subprocess (linked with gdkpixbuf) that would do the job with stdin/stdout?

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Stefan Hajnoczi]

On Tue, Mar 13, 2012 at 11:14 AM, Marc-André Lureau <mlureau@redhat.com> wrote:
> Hi
>
> ----- Mensaje original -----
>> Yeah it wants png, jpeg, tiff.  What I'm concerned about is that QEMU
>> should be doing emulation/virtualization, not processing images for
>> an
>> end-user.  Its main loop is not structured to do utility tasks
>> unrelated to running the guest.  If implemented properly QEMU isn't a
>> good place to host this processing, and if done badly we block the
>> guest and make it unresponsive.  This is a general architectural
>> problem, not something specific to your proposal but this is why I'm
>> against putting this into QEMU.
>>
>> If you send a ppm over a file descriptor, what's the fundamental
>> limitation which requires you to add this code to QEMU?  Perhaps
>> simply supporting file descriptors is the right level of support from
>> QEMU.
>
> That sounds reasonable to me, I would even remove the PPM support, and the format conversion in this case.
>
> It needs to be done somewhere though, and it seems reasonable to do it from the source.
>
> What if we fork a subprocess (linked with gdkpixbuf) that would do the job with stdin/stdout?

If you want to do it as part of the QEMU codebase then a thread is
probably the best way - it avoids the troubles of forking a
multithreaded program and letting go of resources (guest memory, file
descriptors) that aren't needed across fork.

Stefan

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Gerd Hoffmann]

  Hi,

> If you want to do it as part of the QEMU codebase then a thread is
> probably the best way - it avoids the troubles of forking a
> multithreaded program and letting go of resources (guest memory, file
> descriptors) that aren't needed across fork.

That pretty much requires async monitor command support though, so the
iothread can continue driving guest i/o while the new worker thread
scales/compresses/writes the screendump.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Stefan Hajnoczi]

On Tue, Mar 13, 2012 at 1:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>  Hi,
>
>> If you want to do it as part of the QEMU codebase then a thread is
>> probably the best way - it avoids the troubles of forking a
>> multithreaded program and letting go of resources (guest memory, file
>> descriptors) that aren't needed across fork.
>
> That pretty much requires async monitor command support though, so the
> iothread can continue driving guest i/o while the new worker thread
> scales/compresses/writes the screendump.

The most practical first step would be simply sending the ppm over a
socket from ppm_save().  The 'screendump' command today already blocks
so no new badness is being added.  There would be no threads or fancy
image encoding.

Stefan

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Gerd Hoffmann]

  Hi,

> The most practical first step would be simply sending the ppm over a
> socket from ppm_save().  The 'screendump' command today already blocks
> so no new badness is being added.  There would be no threads or fancy
> image encoding.

Adding scaling / compression support will add more overhead though, so
doing that without offloading screendump to a thread first is a bad idea
I think.  Or at least have some numbers to see how bad it actually is.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Stefan Hajnoczi]

On Wed, Mar 14, 2012 at 9:51 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>  Hi,
>
>> The most practical first step would be simply sending the ppm over a
>> socket from ppm_save().  The 'screendump' command today already blocks
>> so no new badness is being added.  There would be no threads or fancy
>> image encoding.
>
> Adding scaling / compression support will add more overhead though, so
> doing that without offloading screendump to a thread first is a bad idea
> I think.  Or at least have some numbers to see how bad it actually is.

I agree, that's why I suggest sending the ppm over a socket.  It
transports out the image data and QEMU itself doesn't do the
encoding/scaling.

Stefan

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Kevin Wolf]

Am 14.03.2012 10:42, schrieb Stefan Hajnoczi:
> On Tue, Mar 13, 2012 at 1:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>  Hi,
>>
>>> If you want to do it as part of the QEMU codebase then a thread is
>>> probably the best way - it avoids the troubles of forking a
>>> multithreaded program and letting go of resources (guest memory, file
>>> descriptors) that aren't needed across fork.
>>
>> That pretty much requires async monitor command support though, so the
>> iothread can continue driving guest i/o while the new worker thread
>> scales/compresses/writes the screendump.
> 
> The most practical first step would be simply sending the ppm over a
> socket from ppm_save().  The 'screendump' command today already blocks
> so no new badness is being added.  There would be no threads or fancy
> image encoding.

Saving PNGs would be useful even without a management tool.

Kevin

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Luiz Capitulino]

On Wed, 14 Mar 2012 10:01:15 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Mar 14, 2012 at 9:51 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >  Hi,
> >
> >> The most practical first step would be simply sending the ppm over a
> >> socket from ppm_save().  The 'screendump' command today already blocks
> >> so no new badness is being added.  There would be no threads or fancy
> >> image encoding.
> >
> > Adding scaling / compression support will add more overhead though, so
> > doing that without offloading screendump to a thread first is a bad idea
> > I think.  Or at least have some numbers to see how bad it actually is.
> 
> I agree, that's why I suggest sending the ppm over a socket.  It
> transports out the image data and QEMU itself doesn't do the
> encoding/scaling.

Just out of curiosity, are we planning to extend the current screendump
command or add a new one?

If we just extend the current command, then we'll have to make the
"filename" parameter optional. But this is a compatible change, I think.

Also, when returning the image via fd, we could offload its writing to
a bh. This would give us a poor man's async support, where the biggest
drawback would the lack of error detection (can this operation fail anyway?).

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Luiz Capitulino]

On Wed, 14 Mar 2012 12:42:36 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 14.03.2012 10:42, schrieb Stefan Hajnoczi:
> > On Tue, Mar 13, 2012 at 1:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>  Hi,
> >>
> >>> If you want to do it as part of the QEMU codebase then a thread is
> >>> probably the best way - it avoids the troubles of forking a
> >>> multithreaded program and letting go of resources (guest memory, file
> >>> descriptors) that aren't needed across fork.
> >>
> >> That pretty much requires async monitor command support though, so the
> >> iothread can continue driving guest i/o while the new worker thread
> >> scales/compresses/writes the screendump.
> > 
> > The most practical first step would be simply sending the ppm over a
> > socket from ppm_save().  The 'screendump' command today already blocks
> > so no new badness is being added.  There would be no threads or fancy
> > image encoding.
> 
> Saving PNGs would be useful even without a management tool.

From HMP you mean? This should be possible, the HMP implementation could
save the image received via fd to a file.

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Alon Levy]

On Wed, Mar 14, 2012 at 10:13:19AM -0300, Luiz Capitulino wrote:
> On Wed, 14 Mar 2012 10:01:15 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Wed, Mar 14, 2012 at 9:51 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >  Hi,
> > >
> > >> The most practical first step would be simply sending the ppm over a
> > >> socket from ppm_save().  The 'screendump' command today already blocks
> > >> so no new badness is being added.  There would be no threads or fancy
> > >> image encoding.
> > >
> > > Adding scaling / compression support will add more overhead though, so
> > > doing that without offloading screendump to a thread first is a bad idea
> > > I think.  Or at least have some numbers to see how bad it actually is.
> > 
> > I agree, that's why I suggest sending the ppm over a socket.  It
> > transports out the image data and QEMU itself doesn't do the
> > encoding/scaling.
> 
> Just out of curiosity, are we planning to extend the current screendump
> command or add a new one?
> 
> If we just extend the current command, then we'll have to make the
> "filename" parameter optional. But this is a compatible change, I think.
> 
> Also, when returning the image via fd, we could offload its writing to
> a bh. This would give us a poor man's async support, where the biggest
> drawback would the lack of error detection (can this operation fail anyway?).
> 

It leaves detection of completion up to the user, so inotify / parsing
the file to see if it's complete (header says size X, but file is still
Y<X, not done..), which looks like as big a drawback.

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On 03/14/2012 07:19 AM, Alon Levy wrote:

>> Also, when returning the image via fd, we could offload its writing to
>> a bh. This would give us a poor man's async support, where the biggest
>> drawback would the lack of error detection (can this operation fail anyway?).
>>
> 
> It leaves detection of completion up to the user, so inotify / parsing
> the file to see if it's complete (header says size X, but file is still
> Y<X, not done..), which looks like as big a drawback.

Notification on a pipe is easy - the reader gets EOF when the writer
closes the fd, with no need to use inotify.  The only remaining issue is
whether the writer closes the fd early on error, so that the reader gets
an incomplete file.  But then the client can tell after getting EOF
whether the image file is complete (read the header, final file size is
too small), without having to have the intermediate management app
parsing headers during the streaming, although it would be a bonus if
there were a way to tell the management app that an error occurred and
qemu closed the write end early.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH/RFC 0/7] Screendump to UNIX socket & in PNG format

[Luiz Capitulino]

On Wed, 14 Mar 2012 07:28:44 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/14/2012 07:19 AM, Alon Levy wrote:
> 
> >> Also, when returning the image via fd, we could offload its writing to
> >> a bh. This would give us a poor man's async support, where the biggest
> >> drawback would the lack of error detection (can this operation fail anyway?).
> >>
> > 
> > It leaves detection of completion up to the user, so inotify / parsing
> > the file to see if it's complete (header says size X, but file is still
> > Y<X, not done..), which looks like as big a drawback.
> 
> Notification on a pipe is easy - the reader gets EOF when the writer
> closes the fd, with no need to use inotify. 

Oh, so I was right, it's a pipe. Alon confused me for a few seconds :-)

> The only remaining issue is
> whether the writer closes the fd early on error, so that the reader gets
> an incomplete file.  But then the client can tell after getting EOF
> whether the image file is complete (read the header, final file size is
> too small), without having to have the intermediate management app
> parsing headers during the streaming, although it would be a bonus if
> there were a way to tell the management app that an error occurred and
> qemu closed the write end early.

This is async support. Anything other than that is going to be a hack
(which can or can not be accepted).