>
> 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?
>
Well, this code is just used for debugging but I would prefer
if this function returns error (bool or whatever) and caller
decide to output some logs. But is just my opinion.
> > +} 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
>
Make sense. For the time being was a proposal.
I have a new version which address also endianess issues!
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel