Patchwork Check fread() results to avoid gcc 4.6 warnings

login
register
mail settings
Submitter David Gibson
Date Aug. 1, 2011, 6:49 a.m.
Message ID <1312181399-29841-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/107682/
State New
Headers show

Comments

David Gibson - Aug. 1, 2011, 6:49 a.m.
When compiling with gcc 4.6, some code in fw_cfg.c complains that fop_ret
is assigned but not used (which is true).  However, it looks like the
meaningless assignments to fop_ret were done to suppress other gcc warnings
due to the fact that fread() is labelled as warn_unused_result in glibc.

This patch avoids both errors, by actually checking the fread() result code
and dropping out with an error message if it fails.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/fw_cfg.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
Stefan Hajnoczi - Aug. 1, 2011, 10:19 a.m.
On Mon, Aug 1, 2011 at 7:49 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> When compiling with gcc 4.6, some code in fw_cfg.c complains that fop_ret
> is assigned but not used (which is true).  However, it looks like the
> meaningless assignments to fop_ret were done to suppress other gcc warnings
> due to the fact that fread() is labelled as warn_unused_result in glibc.
>
> This patch avoids both errors, by actually checking the fread() result code
> and dropping out with an error message if it fails.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/fw_cfg.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Stefan Hajnoczi - Aug. 3, 2011, 10:15 a.m.
On Mon, Aug 01, 2011 at 11:19:38AM +0100, Stefan Hajnoczi wrote:
> On Mon, Aug 1, 2011 at 7:49 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > When compiling with gcc 4.6, some code in fw_cfg.c complains that fop_ret
> > is assigned but not used (which is true).  However, it looks like the
> > meaningless assignments to fop_ret were done to suppress other gcc warnings
> > due to the fact that fread() is labelled as warn_unused_result in glibc.
> >
> > This patch avoids both errors, by actually checking the fread() result code
> > and dropping out with an error message if it fails.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/fw_cfg.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

I'm not taking this into the trivial-patches tree because a build fix
like this should be merged sooner.  With trivial-patches I only send one
pull request per week and in the meantime people would have to
./configure --disable-werror which isn't good.

Stefan
Stefan Berger - Aug. 3, 2011, 1:46 p.m.
On 08/01/2011 06:19 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 1, 2011 at 7:49 AM, David Gibson
> <david@gibson.dropbear.id.au>  wrote:
>> When compiling with gcc 4.6, some code in fw_cfg.c complains that fop_ret
>> is assigned but not used (which is true).  However, it looks like the
>> meaningless assignments to fop_ret were done to suppress other gcc warnings
>> due to the fact that fread() is labelled as warn_unused_result in glibc.
>>
>> This patch avoids both errors, by actually checking the fread() result code
>> and dropping out with an error message if it fails.
>>
>> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
>> ---
>>   hw/fw_cfg.c |   13 +++++++++++++
>>   1 files changed, 13 insertions(+), 0 deletions(-)
> Reviewed-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>
Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Anthony Liguori - Aug. 5, 2011, 4:46 p.m.
On 08/01/2011 01:49 AM, David Gibson wrote:
> When compiling with gcc 4.6, some code in fw_cfg.c complains that fop_ret
> is assigned but not used (which is true).  However, it looks like the
> meaningless assignments to fop_ret were done to suppress other gcc warnings
> due to the fact that fread() is labelled as warn_unused_result in glibc.
>
> This patch avoids both errors, by actually checking the fread() result code
> and dropping out with an error message if it fails.
>
> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/fw_cfg.c |   13 +++++++++++++
>   1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index a29db90..e4847b7 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -87,6 +87,13 @@ 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);
> +    if (fop_ret != 2) {
> +        error_report("Could not read header from '%s': %s",
> +                     filename, strerror(errno));
> +        fclose(fp);
> +        fp = NULL;
> +        return fp;
> +    }
>       filehead_value = (buf[0] + (buf[1]<<  8))&  0xffff;
>       if (filehead_value == 0xd8ff) {
>           file_type = JPG_FILE;
> @@ -181,6 +188,12 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>           boot_splash_filedata_size = file_size;
>           fseek(fp, 0L, SEEK_SET);
>           fop_ret = fread(boot_splash_filedata, 1, file_size, fp);
> +        if (fop_ret != file_size) {
> +            error_report("failed to read data from '%s'.",
> +                         boot_splash_filename);
> +            fclose(fp);
> +            return;
> +        }
>           fclose(fp);
>           /* insert data */
>           if (file_type == JPG_FILE) {

Patch

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index a29db90..e4847b7 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -87,6 +87,13 @@  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);
+    if (fop_ret != 2) {
+        error_report("Could not read header from '%s': %s",
+                     filename, strerror(errno));
+        fclose(fp);
+        fp = NULL;
+        return fp;
+    }
     filehead_value = (buf[0] + (buf[1] << 8)) & 0xffff;
     if (filehead_value == 0xd8ff) {
         file_type = JPG_FILE;
@@ -181,6 +188,12 @@  static void fw_cfg_bootsplash(FWCfgState *s)
         boot_splash_filedata_size = file_size;
         fseek(fp, 0L, SEEK_SET);
         fop_ret = fread(boot_splash_filedata, 1, file_size, fp);
+        if (fop_ret != file_size) {
+            error_report("failed to read data from '%s'.",
+                         boot_splash_filename);
+            fclose(fp);
+            return;
+        }
         fclose(fp);
         /* insert data */
         if (file_type == JPG_FILE) {