Hi,
On Thu, Dec 17, 2015 at 12:49:14PM -0500, Frediano Ziglio wrote:
> >
> > spice-bitmap-utils.c has a lot of calls to fwrite() that don't check the
> > return value. On RHEL 6 these cause compilation errors due to -Werror:
> >
> > spice-bitmap-utils.c:147: error: ignoring return value of 'fwrite',
> > declared with attribute warn_unused_result
> >
> > What should be done about that?
> > * One option would be to remove -Werror but that would be really
> > heavy handed.
> >
> > * I have not found a way to disable just that one warning. But if
> > others have more luck that could be an option.
> >
> > * The best solution would be to not ignore the return values but that
> > means adding a lot of error-handling code. Does anyone want to take
> > this on?
> >
> > * Otherwise the usual way to trick the compiler is to rewrite the code
> > as
> > (void)(func(...)+1)
> > It's really ugly so it is generally further hidden in a macro.
> > This would yield a patch somewhat like this:
> >
> >
> > diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
> > index 8d6e7c6..3cc8b35 100644
> > --- a/server/spice-bitmap-utils.c
> > +++ b/server/spice-bitmap-utils.c
> > @@ -30,6 +30,8 @@
> > #define GRADUAL_HIGH_RGB24_TH -0.03
> > #define GRADUAL_HIGH_RGB16_TH 0
> >
> > +#define IGNORE(x) ((void)(x+1))
> > +
> > // setting a more permissive threshold for stream identification in order
> > // not to miss streams that were artificially scaled on the guest (e.g.,
> > full screen view
> > // in window media player 12). see red_stream_add_frame
> > @@ -141,7 +143,7 @@ static void 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);
> > + IGNORE(fwrite(plt->ents + i, sizeof(uint32_t), 1, f));
> > }
> > }
> >
> > @@ -150,11 +152,11 @@ static void dump_line(FILE *f, uint8_t* line, uint16_t
> > n_pixel_bits, int width,
> > int i;
> > int copy_bytes_size = SPICE_ALIGN(n_pixel_bits * width, 8) / 8;
> >
> > - fwrite(line, 1, copy_bytes_size, f);
> > + IGNORE(fwrite(line, 1, copy_bytes_size, f));
> > if (row_size > copy_bytes_size) {
> > // each line should be 4 bytes aligned
> > for (i = copy_bytes_size; i < row_size; i++) {
> > - fprintf(f, "%c", 0);
> > + IGNORE(fprintf(f, "%c", 0));
> > }
> > }
> > }
> > @@ -235,36 +237,36 @@ void dump_bitmap(SpiceBitmap *bitmap)
> > }
> >
> > /* writing the bmp v3 header */
> > - fprintf(f, "BM");
> > - fwrite(&file_size, sizeof(file_size), 1, f);
> > + IGNORE(fprintf(f, "BM"));
> > + IGNORE(fwrite(&file_size, sizeof(file_size), 1, f));
> > tmp_u16 = alpha ? 1 : 0;
> > - fwrite(&tmp_u16, sizeof(tmp_u16), 1, f); // reserved for application
> > + IGNORE(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);
> > + IGNORE(fwrite(&tmp_u16, sizeof(tmp_u16), 1, f));
> > + IGNORE(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
> > + IGNORE(fwrite(&tmp_u32, sizeof(tmp_u32), 1, f)); // sub header size
> > tmp_32 = bitmap->x;
> > - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > + IGNORE(fwrite(&tmp_32, sizeof(tmp_32), 1, f));
> > tmp_32 = bitmap->y;
> > - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> > + IGNORE(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
> > + IGNORE(fwrite(&tmp_u16, sizeof(tmp_u16), 1, f)); // color plane
> > + IGNORE(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
> > + IGNORE(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
> > + IGNORE(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);
> > + IGNORE(fwrite(&tmp_32, sizeof(tmp_32), 1, f));
> > + IGNORE(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);
> > + IGNORE(fwrite(&tmp_u32, sizeof(tmp_u32), 1, f));
> > tmp_u32 = 0;
> > - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f);
> > + IGNORE(fwrite(&tmp_u32, sizeof(tmp_u32), 1, f));
> >
> > if (plt) {
> > dump_palette(f, plt);
>
> Other proposal, handle the error
Yeah, handling the error looks better ..
>
>
> diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
> index 8d6e7c6..23644d9 100644
> --- a/server/spice-bitmap-utils.c
> +++ b/server/spice-bitmap-utils.c
> @@ -137,27 +137,37 @@ int spice_bitmap_from_surface_type(uint32_t surface_format)
>
> #define RAM_PATH "/tmp/tmpfs"
>
> -static void dump_palette(FILE *f, SpicePalette* plt)
> +#define write(buf, size) do { \
> + if (fwrite(buf, 1, (size), f) != (size)) \
> + goto write_err; \
Do you think some debugging in case of error could help?
> +} while(0)
> +
> +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)
> {
> - int i;
> + static char zeroes[4] = { 0 };
> int copy_bytes_size = SPICE_ALIGN(n_pixel_bits * width, 8) / 8;
>
> - fwrite(line, 1, copy_bytes_size, f);
> - if (row_size > copy_bytes_size) {
> - // each line should be 4 bytes aligned
> - for (i = copy_bytes_size; i < row_size; i++) {
> - fprintf(f, "%c", 0);
> - }
> - }
> + write(line, copy_bytes_size);
> + // each line should be 4 bytes aligned
> + write(zeroes, row_size - copy_bytes_size);
> + return true;
> +
> +write_err:
> + return false;
> }
> +
> void dump_bitmap(SpiceBitmap *bitmap)
> {
> static uint32_t file_id = 0;
> @@ -235,47 +245,51 @@ void dump_bitmap(SpiceBitmap *bitmap)
> }
>
> /* writing the bmp v3 header */
> - fprintf(f, "BM");
> - fwrite(&file_size, sizeof(file_size), 1, f);
> + write("BM", 2);
> + write(&file_size, sizeof(file_size));
> tmp_u16 = alpha ? 1 : 0;
> - fwrite(&tmp_u16, sizeof(tmp_u16), 1, f); // reserved for application
> + write(&tmp_u16, sizeof(tmp_u16)); // reserved for application
> tmp_u16 = 0;
> - fwrite(&tmp_u16, sizeof(tmp_u16), 1, f);
> - fwrite(&bitmap_data_offset, sizeof(bitmap_data_offset), 1, f);
> + write(&tmp_u16, sizeof(tmp_u16));
> + write(&bitmap_data_offset, sizeof(bitmap_data_offset));
> tmp_u32 = header_size - 14;
> - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // sub header size
> + write(&tmp_u32, sizeof(tmp_u32)); // sub header size
> tmp_32 = bitmap->x;
> - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> + write(&tmp_32, sizeof(tmp_32));
> tmp_32 = bitmap->y;
> - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> + write(&tmp_32, sizeof(tmp_32));
>
> 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
> + write(&tmp_u16, sizeof(tmp_u16)); // color plane
> + write(&n_pixel_bits, sizeof(n_pixel_bits)); // pixel depth
>
> tmp_u32 = 0;
> - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // compression method
> + write(&tmp_u32, sizeof(tmp_u32)); // compression method
>
> tmp_u32 = 0; //file_size - bitmap_data_offset;
> - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f); // image size
> + write(&tmp_u32, sizeof(tmp_u32)); // image size
> tmp_32 = 0;
> - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> - fwrite(&tmp_32, sizeof(tmp_32), 1, f);
> + write(&tmp_32, sizeof(tmp_32));
> + write(&tmp_32, sizeof(tmp_32));
> tmp_u32 = (!plt) ? 0 : plt->num_ents; // plt entries
> - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f);
> + write(&tmp_u32, sizeof(tmp_u32));
> tmp_u32 = 0;
> - fwrite(&tmp_u32, sizeof(tmp_u32), 1, f);
> + write(&tmp_u32, sizeof(tmp_u32));
>
> 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;
> }
> }
> +
> +write_err:
> fclose(f);
> }
>
and #undef write
>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel