linux-kernel - Re: [RFC] perf/jevents: Add new structure to pass json fields.

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]

Message-ID: <db9dd8b3-e2ca-6773-a8bb-0f635eb84953@linux.ibm.com>
Date:   Thu, 27 Aug 2020 18:27:17 +0530
From:   kajoljain <kjain@...ux.ibm.com>
To:     John Garry <john.garry@...wei.com>, Jiri Olsa <jolsa@...hat.com>
Cc:     acme@...nel.org, ak@...ux.intel.com, yao.jin@...ux.intel.com,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        irogers@...gle.com, maddy@...ux.ibm.com,
        ravi.bangoria@...ux.ibm.com
Subject: Re: [RFC] perf/jevents: Add new structure to pass json fields.



On 8/26/20 5:03 PM, John Garry wrote:
> On 26/08/2020 12:24, kajoljain wrote:
>>
>>
>> On 8/26/20 4:30 PM, Jiri Olsa wrote:
>>> On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote:
>>>
>>> SNIP
>>>
>>>>>                    goto free_strings;
>>>>>            }
>>>>> -        err = func(data, name, real_event(name, event), desc, long_desc,
>>>>> -               pmu, unit, perpkg, metric_expr, metric_name,
>>>>> -               metric_group, deprecated, metric_constraint);
>>>>> +        je->event = real_event(je->name, je->event);
>>>>> +        err = func(data, je);
>>>>>    free_strings:
>>>>> -        free(event);
>>>>> -        free(desc);
>>>>> -        free(name);
>>>>> -        free(long_desc);
>>>>>            free(extra_desc);
>>>>> -        free(pmu);
>>>>>            free(filter);
>>>>> -        free(perpkg);
>>>>> -        free(deprecated);
>>>>> -        free(unit);
>>>>> -        free(metric_expr);
>>>>> -        free(metric_name);
>>>>> -        free(metric_group);
>>>>> -        free(metric_constraint);
> 
> Hi Kajol Jain,
> 
> Do we need to free je->metric_name and the rest still? From a glance, that memory is still separately alloc'ed in addfield.

Hi John,
    yes right we should free them as well. Thanks for pointing it, I will update.

Thanks,
Kajol Jain
> 
>>>>>            free(arch_std);
>>>>> +        free(je);
>>>>>            if (err)
>>>>>                break;
>>>>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>>>>> index 2afc8304529e..e696edf70e9a 100644
>>>>> --- a/tools/perf/pmu-events/jevents.h
>>>>> +++ b/tools/perf/pmu-events/jevents.h
>>>>
>>>> Somewhat unrelated - this file only seems to be included in jevents.c, so I
>>>> don't see why it exists...
>>>
>>> ah right.. I won't mind getting rid of it
>>
>> Hi John and  Jiri
>>       Thanks for reviewing the patch. I can remove this file and add these structure inside jevents.c
> 
> thanks
> 
>>
>> Thanks,
>> Kajol Jain
>>>  
>>>>> @@ -2,14 +2,28 @@
>>>>>    #ifndef JEVENTS_H
>>>>>    #define JEVENTS_H 1
>>>>> +#include "pmu-events.h"
>>>>> +
>>>>> +struct json_event {
>>>>> +    char *name;
>>>>> +    char *event;
>>>>> +    char *desc;
>>>>> +    char *topic;
>>>>> +    char *long_desc;
>>>>> +    char *pmu;
>>>>> +    char *unit;
>>>>> +    char *perpkg;
>>>>> +    char *metric_expr;
>>>>> +    char *metric_name;
>>>>> +    char *metric_group;
>>>>> +    char *deprecated;
>>>>> +    char *metric_constraint;
>>>>
>>>> This looks very much like struct event_struct, so could look to consolidate:
>>>>
>>>> struct event_struct {
>>>>     struct list_head list;
>>>>     char *name;
>>>>     char *event;
>>>>     char *desc;
>>>>     char *long_desc;
>>>>     char *pmu;
>>>>     char *unit;
>>>>     char *perpkg;
>>>>     char *metric_expr;
>>>>     char *metric_name;
>>>>     char *metric_group;
>>>>     char *deprecated;
>>>>     char *metric_constraint;
>>>> };
>>>
>>> as Andi said they come from different layers, I think it's
>>> better to keep them separated even if they share some fields
> 
> I was just suggesting to make:
>  struct event_struct {
>     struct list_head list;
>     struct json_event je;
>  }
> 
> No biggie if against this.
> 
> Cheers,
> John