[Spice-devel] [PATCH 2/2] utils: fix endianess issues writing bitmap file and handle errors

[Frediano Ziglio]

> 
> On Mon, 2015-12-21 at 11:28 +0000, Frediano Ziglio wrote:
> > Do not print directly binary data but serialize in a portable way
> > and then use write functions.
> > Also handle errors writing to file.
> > ---
> >  server/spice-bitmap-utils.c | 106
> >  +++++++++++++++++++++++++++----------------
> > -
> >  1 file changed, 66 insertions(+), 40 deletions(-)
> > 
> > diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
> > index 21d43bf..559b0de 100644
> > --- a/server/spice-bitmap-utils.c
> > +++ b/server/spice-bitmap-utils.c
> > @@ -137,22 +137,51 @@ int spice_bitmap_from_surface_type(uint32_t
> > surface_format)
> >  
> >  #define RAM_PATH "/tmp/tmpfs"
> >  
> > -static void dump_palette(FILE *f, SpicePalette* plt)
> > +static void put_16le(uint8_t **ptr, uint16_t val)
> > +{
> > +    **ptr = val & 0xffu;
> > +    (*ptr)++;
> > +    val >>= 8;
> > +    **ptr = val & 0xffu;
> > +    (*ptr)++;
> > +}
> > +
> > +static void put_32le(uint8_t **ptr, uint32_t val)
> > +{
> > +    put_16le(ptr, val & 0xffffu);
> > +    val >>= 16;
> > +    put_16le(ptr, val & 0xffffu);
> > +}
> > +
> > +#define write(buf, size) do { \
> > +    if (fwrite(buf, 1, (size), f) != (size)) \
> > +        goto write_err; \
> > +} while(0)
> 
> I'd personally prefer that the FILE pointer was an argument to the macro and
> we
> didn't assume a variable named 'f'.
> 

Done

> > +
> > +static bool dump_palette(FILE *f, SpicePalette* plt)
> >  {
> >      int i;
> >      for (i = 0; i < plt->num_ents; i++) {
> > -        fwrite(plt->ents + i, sizeof(uint32_t), 1, f);
> > +        write(plt->ents + i, sizeof(uint32_t));
> >      }
> > +    return true;
> > +
> > +write_err:
> > +    return false;
> >  }
> >  
> > -static void dump_line(FILE *f, uint8_t* line, uint16_t n_pixel_bits, int
> > width, int row_size)
> > +static bool dump_line(FILE *f, uint8_t* line, uint16_t n_pixel_bits, int
> > width, int row_size)
> >  {
> >      static char zeroes[4] = { 0 };
> >      int copy_bytes_size = SPICE_ALIGN(n_pixel_bits * width, 8) / 8;
> >  
> > -    fwrite(line, 1, copy_bytes_size, f);
> > +    write(line, copy_bytes_size);
> >      // each line should be 4 bytes aligned
> > -    fwrite(zeroes, 1, row_size - copy_bytes_size, f);
> > +    write(zeroes, row_size - copy_bytes_size);
> > +    return true;
> > +
> > +write_err:
> > +    return false;
> >  }
> >  
> >  void dump_bitmap(SpiceBitmap *bitmap)
> > @@ -169,11 +198,9 @@ void dump_bitmap(SpiceBitmap *bitmap)
> >      int alpha = 0;
> >      uint32_t header_size = 14 + 40;
> >      uint32_t bitmap_data_offset;
> > -    uint32_t tmp_u32;
> > -    int32_t tmp_32;
> > -    uint16_t tmp_u16;
> >      FILE *f;
> >      int i, j;
> > +    uint8_t header[128], *ptr;
> 
> Out of curiosity, where does the 128 come from? Just an arbitrary value
> that's
> plenty large for the header?
> 

Yes, large enough.

> >  
> >      switch (bitmap->format) {
> >      case SPICE_BITMAP_FMT_1BIT_BE:
> > @@ -232,47 +259,46 @@ void dump_bitmap(SpiceBitmap *bitmap)
> >      }
> >  
> >      /* writing the bmp v3 header */
> > -    fwrite("BM", 2, 1, f);
> > -    fwrite(&file_size, sizeof(file_size), 1, f);
> > -    tmp_u16 = alpha ? 1 : 0;
> > -    fwrite(&tmp_u16, sizeof(tmp_u16), 1, f); // reserved for application
> > -    tmp_u16 = 0;
> > -    fwrite(&tmp_u16, sizeof(tmp_u16), 1, f);
> > -    fwrite(&bitmap_data_offset, sizeof(bitmap_data_offset), 1, f);
> > -    tmp_u32 = header_size - 14;
> > -    fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // sub header size
> > -    tmp_32 = bitmap->x;
> > -    fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > -    tmp_32 = bitmap->y;
> > -    fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > -
> > -    tmp_u16 = 1;
> > -    fwrite(&tmp_u16, sizeof(tmp_u16), 1, f); // color plane
> > -    fwrite(&n_pixel_bits, sizeof(n_pixel_bits), 1, f); // pixel depth
> > -
> > -    tmp_u32 = 0;
> > -    fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // compression method
> > -
> > -    tmp_u32 = 0; //file_size - bitmap_data_offset;
> > -    fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // image size
> > -    tmp_32 = 0;
> > -    fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > -    fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > -    tmp_u32 = (!plt) ? 0 : plt->num_ents; // plt entries
> > -    fwrite(&tmp_u32, sizeof(tmp_u32), 1, f);
> > -    tmp_u32 = 0;
> > -    fwrite(&tmp_u32, sizeof(tmp_u32), 1, f);
> > +    ptr = header;
> > +    put_16le(&ptr, 0x4D42); // "BM"
> > +    put_32le(&ptr, file_size);
> > +    put_16le(&ptr, alpha);
> > +    put_16le(&ptr, 0);
> > +    put_32le(&ptr, bitmap_data_offset);
> > +    put_32le(&ptr, header_size - 14);
> > +    put_32le(&ptr, bitmap->x);
> > +    put_32le(&ptr, bitmap->y);
> > +
> > +    put_16le(&ptr, 1); // plane
> > +    put_16le(&ptr, n_pixel_bits);
> > +
> > +    put_32le(&ptr, 0); // compression
> > +
> > +    put_32le(&ptr, 0); //file_size - bitmap_data_offset;
> > +    put_32le(&ptr, 0);
> > +    put_32le(&ptr, 0);
> > +    put_32le(&ptr, !plt ? 0 : plt->num_ents); // plt entries
> > +    put_32le(&ptr, 0);
> > +
> > +    write(header, ptr - header);
> >  
> >      if (plt) {
> > -        dump_palette(f, plt);
> > +        if (!dump_palette(f, plt))
> > +            goto write_err;
> >      }
> >      /* writing the data */
> >      for (i = 0; i < bitmap->data->num_chunks; i++) {
> >          SpiceChunk *chunk = &bitmap->data->chunk[i];
> >          int num_lines = chunk->len / bitmap->stride;
> >          for (j = 0; j < num_lines; j++) {
> > -            dump_line(f, chunk->data + (j * bitmap->stride), n_pixel_bits,
> > bitmap->x, row_size);
> > +            if (!dump_line(f, chunk->data + (j * bitmap->stride),
> > n_pixel_bits, bitmap->x, row_size))
> > +                goto write_err;
> >          }
> >      }
> >      fclose(f);
> > +    return;
> > +
> > +write_err:
> > +    fclose(f);
> > +    remove(file_str);
> >  }
> 
> Other than the minor comments above, looks fine to me
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> 

Frediano