Message ID | 1311967824-5218-1-git-send-email-weil@mail.berlios.de |
---|---|
State | Superseded |
Headers | show |
On 29 July 2011 20:30, Stefan Weil <weil@mail.berlios.de> wrote: > Commit 3d3b8303c6f83b9b245bc774af530a6403cc4ce6 > breaks builds with gcc-4.6: > > hw/fw_cfg.c: In function ‘probe_splashfile’: > hw/fw_cfg.c:66:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] > hw/fw_cfg.c: In function ‘fw_cfg_bootsplash’: > hw/fw_cfg.c:130:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] > > Remove fop_ret. Testing the result of fread() is normally > a good idea, but I don't think it is needed here. > @@ -86,7 +85,7 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep) > } > /* check magic ID */ > fseek(fp, 0L, SEEK_SET); > - fop_ret = fread(buf, 1, 2, fp); > + (void)fread(buf, 1, 2, fp); Usually this kind of thing is added in order to stop gcc complaining about you ignoring the return value of a function which has been marked (by libc) as 'don't-ignore-return-value'. In such cases a "(void)" is not sufficient to suppress the "return value ignored" warning. At least some of these cases really should be checking fread return values; I see we also don't check fseek() return values either in all places. So the easiest fix is just to check all the fread() calls. Alternative suggestion: it would be easier to just slurp the whole file into memory (which is what we do once we've figured out it's an image) and then check the magic numbers in the memory buffer, which removes a lot of these unchecked function calls altogether. Since we're linking against glib anyway it looks like g_file_get_contents() would do 95% of the work for us. [disclaimer: I haven't used that API myself but it looks the right shape...g_malloc vs qemu_malloc issues, maybe?] Failing that, fopen/fstat/fread/fclose/check magic numbers. -- PMM
On 07/29/2011 07:18 PM, Peter Maydell wrote: > On 29 July 2011 20:30, Stefan Weil<weil@mail.berlios.de> wrote: >> Commit 3d3b8303c6f83b9b245bc774af530a6403cc4ce6 >> breaks builds with gcc-4.6: >> >> hw/fw_cfg.c: In function ‘probe_splashfile’: >> hw/fw_cfg.c:66:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] >> hw/fw_cfg.c: In function ‘fw_cfg_bootsplash’: >> hw/fw_cfg.c:130:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] >> >> Remove fop_ret. Testing the result of fread() is normally >> a good idea, but I don't think it is needed here. > >> @@ -86,7 +85,7 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep) >> } >> /* check magic ID */ >> fseek(fp, 0L, SEEK_SET); >> - fop_ret = fread(buf, 1, 2, fp); >> + (void)fread(buf, 1, 2, fp); > > Usually this kind of thing is added in order to stop gcc complaining about > you ignoring the return value of a function which has been marked (by libc) > as 'don't-ignore-return-value'. In such cases a "(void)" is not sufficient > to suppress the "return value ignored" warning. > > At least some of these cases really should be checking fread return values; > I see we also don't check fseek() return values either in all places. So > the easiest fix is just to check all the fread() calls. > > Alternative suggestion: it would be easier to just slurp the whole file > into memory (which is what we do once we've figured out it's an image) > and then check the magic numbers in the memory buffer, which removes > a lot of these unchecked function calls altogether. Since we're linking > against glib anyway it looks like g_file_get_contents() would do 95% > of the work for us. [disclaimer: I haven't used that API myself but it > looks the right shape...g_malloc vs qemu_malloc issues, maybe?] Failing > that, fopen/fstat/fread/fclose/check magic numbers. As long as it's not mixed, it shouldn't be a problem. I think using g_file_get_contents would make sense here. Regards, Anthony Liguori > > -- PMM > >
On Mon, Aug 1, 2011 at 8:30 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 07/29/2011 07:18 PM, Peter Maydell wrote: >> >> On 29 July 2011 20:30, Stefan Weil<weil@mail.berlios.de> wrote: >>> >>> Commit 3d3b8303c6f83b9b245bc774af530a6403cc4ce6 >>> breaks builds with gcc-4.6: >>> >>> hw/fw_cfg.c: In function ‘probe_splashfile’: >>> hw/fw_cfg.c:66:9: error: variable ‘fop_ret’ set but not used >>> [-Werror=unused-but-set-variable] >>> hw/fw_cfg.c: In function ‘fw_cfg_bootsplash’: >>> hw/fw_cfg.c:130:9: error: variable ‘fop_ret’ set but not used >>> [-Werror=unused-but-set-variable] >>> >>> Remove fop_ret. Testing the result of fread() is normally >>> a good idea, but I don't think it is needed here. >> >>> @@ -86,7 +85,7 @@ static FILE *probe_splashfile(char *filename, int >>> *file_sizep, int *file_typep) >>> } >>> /* check magic ID */ >>> fseek(fp, 0L, SEEK_SET); >>> - fop_ret = fread(buf, 1, 2, fp); >>> + (void)fread(buf, 1, 2, fp); >> >> Usually this kind of thing is added in order to stop gcc complaining about >> you ignoring the return value of a function which has been marked (by >> libc) >> as 'don't-ignore-return-value'. In such cases a "(void)" is not sufficient >> to suppress the "return value ignored" warning. >> >> At least some of these cases really should be checking fread return >> values; >> I see we also don't check fseek() return values either in all places. So >> the easiest fix is just to check all the fread() calls. >> >> Alternative suggestion: it would be easier to just slurp the whole file >> into memory (which is what we do once we've figured out it's an image) >> and then check the magic numbers in the memory buffer, which removes >> a lot of these unchecked function calls altogether. Since we're linking >> against glib anyway it looks like g_file_get_contents() would do 95% >> of the work for us. [disclaimer: I haven't used that API myself but it >> looks the right shape...g_malloc vs qemu_malloc issues, maybe?] Failing >> that, fopen/fstat/fread/fclose/check magic numbers. > > As long as it's not mixed, it shouldn't be a problem. > > I think using g_file_get_contents would make sense here. I have also met this problem on fedora 15 today. Currently i disable werror option to build successfully. How to completely this problem regardless of gcc>=4.6? > > Regards, > > Anthony Liguori > >> >> -- PMM >> >> > > >
On 18 August 2011 05:16, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > I have also met this problem on fedora 15 today. Currently i disable > werror option to build successfully. How to completely this problem > regardless of gcc>=4.6? Hmm, this should have been fixed by commit 257a73755. Can you tell us which git revision you are trying to build and quote the complete error message from gcc, please? thanks -- PMM
On Thu, Aug 18, 2011 at 12:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 August 2011 05:16, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: >> I have also met this problem on fedora 15 today. Currently i disable >> werror option to build successfully. How to completely this problem >> regardless of gcc>=4.6? > > Hmm, this should have been fixed by commit 257a73755. Can you > tell us which git revision you are trying to build and quote > the complete error message from gcc, please? Sorry, my branch is a bit old. After rebasing it to latest version, the fix has checked in and this issue has been resolved. thanks. > > thanks > -- PMM >
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c index a29db90..d906b83 100644 --- a/hw/fw_cfg.c +++ b/hw/fw_cfg.c @@ -63,7 +63,6 @@ struct FWCfgState { static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep) { FILE *fp = NULL; - int fop_ret; int file_size; int file_type = -1; unsigned char buf[2] = {0, 0}; @@ -86,7 +85,7 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep) } /* check magic ID */ fseek(fp, 0L, SEEK_SET); - fop_ret = fread(buf, 1, 2, fp); + (void)fread(buf, 1, 2, fp); filehead_value = (buf[0] + (buf[1] << 8)) & 0xffff; if (filehead_value == 0xd8ff) { file_type = JPG_FILE; @@ -105,7 +104,7 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep) /* check BMP bpp */ if (file_type == BMP_FILE) { fseek(fp, 28, SEEK_SET); - fop_ret = fread(buf, 1, 2, fp); + (void)fread(buf, 1, 2, fp); bmp_bpp = (buf[0] + (buf[1] << 8)) & 0xffff; if (bmp_bpp != 24) { error_report("only 24bpp bmp file is supported."); @@ -127,7 +126,6 @@ static void fw_cfg_bootsplash(FWCfgState *s) char *p; char *filename; FILE *fp; - int fop_ret; int file_size; int file_type = -1; const char *temp; @@ -180,7 +178,7 @@ static void fw_cfg_bootsplash(FWCfgState *s) boot_splash_filedata = qemu_malloc(file_size); boot_splash_filedata_size = file_size; fseek(fp, 0L, SEEK_SET); - fop_ret = fread(boot_splash_filedata, 1, file_size, fp); + (void)fread(boot_splash_filedata, 1, file_size, fp); fclose(fp); /* insert data */ if (file_type == JPG_FILE) {
Commit 3d3b8303c6f83b9b245bc774af530a6403cc4ce6 breaks builds with gcc-4.6: hw/fw_cfg.c: In function ‘probe_splashfile’: hw/fw_cfg.c:66:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] hw/fw_cfg.c: In function ‘fw_cfg_bootsplash’: hw/fw_cfg.c:130:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable] Remove fop_ret. Testing the result of fread() is normally a good idea, but I don't think it is needed here. Cc: Wayne Xia <xiawenc@linux.vnet.ibm.com> Cc: Anthony Liguori <aliguori@us.ibm.com> Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- hw/fw_cfg.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)