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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Oct 2020 09:35:28 +0100
From:   John Garry <john.garry@...wei.com>
To:     kajoljain <kjain@...ux.ibm.com>, Namhyung Kim <namhyung@...nel.org>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>, Ian Rogers <irogers@...gle.com>,
        Yao Jin <yao.jin@...ux.intel.com>, <yeyunfeng@...wei.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH 1/2] perf jevents: Tidy error handling

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