[v3,5/5] intel/tools: avoid 'ignoring return value'

Submitted by andrey simiklit on Nov. 14, 2018, 1:49 p.m.

Details

Message ID 20181114134924.21154-1-asimiklit.work@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Nov. 14, 2018, 1:49 p.m.
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

1. tools/i965_disasm.c:58:4: warning:
     ignoring return value of ‘fread’,
     declared with attribute warn_unused_result
     fread(assembly, *end, 1, fp);

v2: Fixed incorrect return value check.
       ( Eric Engestrom <eric.engestrom@intel.com> )

v3: Zero size file check moved before fread with exit()
       ( Eric Engestrom <eric.engestrom@intel.com> )

Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
---
 src/intel/tools/i965_disasm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
index 73a6760fc1..d820b10d97 100644
--- a/src/intel/tools/i965_disasm.c
+++ b/src/intel/tools/i965_disasm.c
@@ -50,12 +50,17 @@  i965_disasm_read_binary(FILE *fp, size_t *end)
    void *assembly;
 
    *end = i965_disasm_get_file_size(fp);
+   if (!*end) {
+      fprintf(stderr, "file is empty\n");
+      exit(0);
+   }
 
    assembly = malloc(*end + 1);
    if (assembly == NULL)
       return NULL;
 
-   fread(assembly, *end, 1, fp);
+   MAYBE_UNUSED size_t size = fread(assembly, *end, 1, fp);
+   assert(size && "error: unable to read all elements!");
    fclose(fp);
 
    return assembly;

Comments

On Wednesday, 2018-11-14 15:49:24 +0200, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> 1. tools/i965_disasm.c:58:4: warning:
>      ignoring return value of ‘fread’,
>      declared with attribute warn_unused_result
>      fread(assembly, *end, 1, fp);
> 
> v2: Fixed incorrect return value check.
>        ( Eric Engestrom <eric.engestrom@intel.com> )
> 
> v3: Zero size file check moved before fread with exit()
>        ( Eric Engestrom <eric.engestrom@intel.com> )
> 
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> ---
>  src/intel/tools/i965_disasm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> index 73a6760fc1..d820b10d97 100644
> --- a/src/intel/tools/i965_disasm.c
> +++ b/src/intel/tools/i965_disasm.c
> @@ -50,12 +50,17 @@ i965_disasm_read_binary(FILE *fp, size_t *end)
>     void *assembly;
>  
>     *end = i965_disasm_get_file_size(fp);
> +   if (!*end) {
> +      fprintf(stderr, "file is empty\n");
> +      exit(0);

Please use EXIT_FAILURE/EXIT_SUCCESS instead of literal values, and
I think this should be a failure not a success (which 0 is).

That said, returning NULL is actually a better behaviour here, as it
allows the caller to clean itself up before exiting.

The commit message should also be changed to something like this (feel
free to copy):
  intel/tools: make sure the binary file is properly read

> +   }
>  
>     assembly = malloc(*end + 1);
>     if (assembly == NULL)
>        return NULL;
>  
> -   fread(assembly, *end, 1, fp);
> +   MAYBE_UNUSED size_t size = fread(assembly, *end, 1, fp);
> +   assert(size && "error: unable to read all elements!");

If we're adding a proper error path above, I think we should do the same
here instead of adding an assert:

  if (!size) {
    free(assembly);
    fclose(fp);
    return NULL;
  }

>     fclose(fp);
>  
>     return assembly;
> -- 
> 2.17.1
>