Re: [PATCH RFC v3 02/12] perf jevents: Add support for system events tables

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

On 11/05/2020 12:01, Jiri Olsa wrote:
> On Thu, May 07, 2020 at 07:57:41PM +0800, John Garry wrote:
> 
> SNIP
> 
>> +				      &sys_event_tables);
>> +		}
>> +
>>   		print_events_table_prefix(eventsfp, tblname);
>>   		return 0;
>>   	}
>> @@ -1180,7 +1253,6 @@ int main(int argc, char *argv[])
>>   	} else if (rc < 0) {
>>   		/* Make build fail */
>>   		fclose(eventsfp);
>> -		free_arch_std_events();
>>   		ret = 1;
>>   		goto out_free_mapfile;
>>   	} else if (rc) {
>> @@ -1206,27 +1278,31 @@ int main(int argc, char *argv[])
>>   	if (close_table)
>>   		print_events_table_suffix(eventsfp);
>>   
>> -	if (!mapfile) {
>> -		pr_info("%s: No CPU->JSON mapping?\n", prog);
>> -		goto empty_map;
>> +	if (mapfile) {
>> +		if (process_mapfile(eventsfp, mapfile)) {
>> +			pr_err("%s: Error processing mapfile %s\n", prog,
>> +			       mapfile);
>> +			/* Make build fail */
>> +			fclose(eventsfp);
>> +			ret = 1;
>> +		}
>> +	} else {
>> +		pr_err("%s: No CPU->JSON mapping?\n", prog);
> 
> shouldn't we jump to empty_map in here? there still needs to be a
> mapfile, right?

In theory we could only support sys events :)

But I'll now make this a (empty map) failure case. And I think that 
another error case handling needs fixing in my patch.


As for this:

  +	fprintf(outfp, "struct pmu_sys_events pmu_sys_event_tables[] = {");
 >> +
 >> +	list_for_each_entry(sys_event_table, &sys_event_tables, list) {
 >> +		fprintf(outfp, "\n\t{\n\t\t.table = %s,\n\t},",
 >> +			sys_event_table->name);
 >> +	}
 >> +	fprintf(outfp, "\n\t{\n\t\t.table = 0\n\t},");
 >
 > this will add extra tabs:
 >
 >          {
 >                  .table = 0
 >          },
 >
 > while the rest of the file starts items without any indent
 >

I'll ensure the indent is the same.

BTW, is there anything to be said for removing the empty map feature 
(and always breaking the perf build instead)? I guess that it was just 
an early feature for dealing with unstable JSONs.

Thanks,
john

> 
> jirka
> 
>>   	}
>>   
>> -	if (process_mapfile(eventsfp, mapfile)) {
>> -		pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
>> -		/* Make build fail */
>> +	if (process_system_event_tables(eventsfp)) {
>>   		fclose(eventsfp);
>> -		free_arch_std_events();
>>   		ret = 1;
>>   	}
>>   
>> -
>>   	goto out_free_mapfile;
>>   
>>   empty_map:
>>   	fclose(eventsfp);
>>   	create_empty_mapping(output_file);
>> -	free_arch_std_events();
>>   out_free_mapfile:
>> +	free_arch_std_events();
>> +	free_sys_event_tables();
>>   	free(mapfile);
>>   	return ret;
>>   }
> 
> SNIP
> 
> .
>