Re: [PATCH 1/2] perf jevents: Tidy error handling

[John Garry <john.garry@huawei.com>]

On 20/10/2020 09:14, kajoljain wrote:
> 
> 
> On 10/20/20 11:13 AM, Namhyung Kim wrote:
>> Hello,
>>
>> On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@huawei.com> wrote:
>>>
>>> There is much duplication in the error handling for directory traversing
>>> for processing JSONs.
>>>
>>> Factor out the common code to tidy a bit.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>> [SNIP]
>>> -empty_map:
>>> +err_processing_std_arch_event_dir:
>>> +       err_string_ext = " for std arch event";
>>> +err_processing_dir:
>>> +       if (verbose || rc > 0) {
>>> +               pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
>>> +                       err_string_ext);
>>
>> This was printed only if verbose is set but now changed.
> 

pr_info() is silenced if verbose is unset, so in effect it is the same - 
see pr_info() definition in json.h

> Hi John,
>     Yes I agree with Namhyung.
> So, this is our original checks
> 
> if (rc && verbose) {
> 		pr_info("%s: Error walking file tree %s\n", prog, ldirname);
> 		goto empty_map;
> 	} else if (rc < 0) {
> 		/* Make build fail */
> 		fclose(eventsfp);
> 		free_arch_std_events();
> 		ret = 1;
> 		goto out_free_mapfile;
> 	} else if (rc) {
> 		goto empty_map;
> 	}
> 
> May be we can use similar checks:
> 
> if( verbose)
>    pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
> if(rc > 0)
>     empty_map = 1;
> else
>    ret = 1;
> 

Not that it matters much, this logic is slightly different for verbose 
set and rc < 0. I don't mind going with that, so let me know.

Thanks for checking,
John

> Thanks,
> Kajol Jain
>    
>>
>> Thanks
>> Namhyung
>>
>>
>>> +               empty_map = 1;
>>> +       } else {
>>> +               ret = 1;
>>> +       }
>>> +err_close_eventsfp:
>>>          fclose(eventsfp);
>>> -       create_empty_mapping(output_file);
>>> +       if (empty_map)
>>> +               create_empty_mapping(output_file);
>>> +err_out:
>>>          free_arch_std_events();
>>> -out_free_mapfile:
>>>          free(mapfile);
>>>          return ret;
>>>   }
>>> --
>>> 2.26.2
>>>
> .
>