Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC] perf/jevents: Add new structure to pass json fields.
To:     John Garry <john.garry@huawei.com>, Jiri Olsa <jolsa@redhat.com>
Cc:     acme@kernel.org, ak@linux.intel.com, yao.jin@linux.intel.com,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        irogers@google.com, maddy@linux.ibm.com,
        ravi.bangoria@linux.ibm.com
References: <20200825074041.378520-1-kjain@linux.ibm.com>
 <bc078472-e859-b7dc-c451-d737dd573edf@huawei.com>
 <20200826110046.GF703542@krava>
 <5d91b929-cffd-265a-dd0c-f63bc3d1565d@linux.ibm.com>
 <ac9acc5f-7ce1-c9c9-91f5-598ca13a4a89@huawei.com>
From:   kajoljain <kjain@linux.ibm.com>
Message-ID: <db9dd8b3-e2ca-6773-a8bb-0f635eb84953@linux.ibm.com>
Date:   Thu, 27 Aug 2020 18:27:17 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <ac9acc5f-7ce1-c9c9-91f5-598ca13a4a89@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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