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

[John Garry]

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.

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