[Mesa-dev] [PATCH v3 5/5] intel/tools: avoid 'ignoring return value'

[Eric Engestrom]

On Wednesday, 2018-11-14 15:49:24 +0200, asimiklit.work at gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit at 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 at intel.com> )
> 
> v3: Zero size file check moved before fread with exit()
>        ( Eric Engestrom <eric.engestrom at intel.com> )
> 
> Signed-off-by: Andrii Simiklit <andrii.simiklit at 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
>