diff mbox

[2/2] vga: ppm_save(): Return error on failure

Message ID 1331740533-26319-2-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy March 14, 2012, 3:55 p.m. UTC 
From: Luiz Capitulino <lcapitulino@redhat.com>

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 <lcapitulino@redhat.com>
---
 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(-)

Comments

Luiz Capitulino March 14, 2012, 5:40 p.m. UTC | #1 
On Wed, 14 Mar 2012 17:55:33 +0200
Alon Levy <alevy@redhat.com> wrote:

> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> 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 <lcapitulino@redhat.com>
> ---
>  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);
>      }
>  }
Alon Levy March 14, 2012, 6:02 p.m. UTC | #2 
On Wed, Mar 14, 2012 at 02:40:42PM -0300, Luiz Capitulino wrote:
> On Wed, 14 Mar 2012 17:55:33 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > From: Luiz Capitulino <lcapitulino@redhat.com>
> > 
> > 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 <lcapitulino@redhat.com>
> > ---
> >  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.

OK, I'll add new QErrors then.

> 
> 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.

Makes sense.

> 
> >          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);
> >      }
> >  }
>
diff mbox

Patch

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);
         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);
     }
 }