lkml.org 
[lkml]   [2020]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] perf jevents: Tidy error handling
From
Date


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@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.

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
>>>>
>> .
>>
>

\
 
 \ /
  Last update: 2020-10-21 15:39    [W:0.107 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site