Re: [Qemu-devel] [PATCH 2/2] vga: ppm_save(): Return error on failure

[Luiz Capitulino]

On Wed, 14 Mar 2012 17:55:33 +0200
Alon Levy <address@hidden> wrote:

> From: Luiz Capitulino <address@hidden>
> 
> This makes all devices using ppm_save() return an error appropriately
> when the screendump command fails.
> 
> Based on a code by Anthony Liguori.
> 
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  hw/blizzard.c   |    2 +-
>  hw/qxl.c        |    2 +-
>  hw/vga.c        |   10 ++++++----
>  hw/vga_int.h    |    3 ++-
>  hw/vmware_vga.c |    2 +-
>  5 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index 76df78c..29e5ae6 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -942,7 +942,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 27f27f5..aa68612 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1503,7 +1503,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 b80a079..7e90cf4 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -165,7 +165,7 @@ static uint16_t expand2[256];
>  static uint8_t expand4to8[16];
>  
>  static void vga_screen_dump(void *opaque, const char *filename, bool cswitch,
> -                            Error *errp);
> +                            Error **errp);
>  
>  static void vga_update_memory_access(VGACommonState *s)
>  {
> @@ -2365,7 +2365,7 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion 
> *system_memory)
>  /********************************************************/
>  /* vga screen dump */
>  
> -int ppm_save(const char *filename, struct DisplaySurface *ds)
> +int ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp)
>  {
>      FILE *f;
>      uint8_t *d, *d1;
> @@ -2377,8 +2377,10 @@ int ppm_save(const char *filename, struct 
> DisplaySurface *ds)
>  
>      trace_ppm_save(filename, ds);
>      f = fopen(filename, "wb");
> -    if (!f)
> +    if (!f) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);

This is a generic error, and it's unfortunate that we're using it in several
places now. The best thing to do here - and I'm including write() in this
discussion - is to report the most possible errors, like EACCESS, ENOSPC,
EPERM, EIO. Maybe not all of them exist as QErrors.

Also, note that some device models have their own way of saving the screen
dump (eg. g364fb_screen_dump()) and should be converted to error_set()
separately.

>          return -1;
> +    }
>      fprintf(f, "P6\n%d %d\n%d\n",
>              ds->width, ds->height, 255);
>      linebuf = g_malloc(ds->width * 3);
> @@ -2420,5 +2422,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..63078ba 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -24,6 +24,7 @@
>  
>  #include <hw/hw.h>
>  #include "memory.h"
> +#include "error.h"
>  
>  #define ST01_V_RETRACE      0x08
>  #define ST01_DISP_ENABLE    0x01
> @@ -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);
> +int 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 6868778..0769652 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1016,7 +1016,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);
>      }
>  }