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

[kajoljain]

On 10/20/20 2:05 PM, John Garry wrote:
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>> [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.

Yes right. Sure if required we can made changes on these checks and include appropriate condition for verbose set and rc < 0. Seems fine to me.

Thanks,
Kajol Jain
> 
> 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
>>>>
>> .
>>
>