[ktap] Re: [PATCH 1/3] symbol: replace find_symbol() by get_dso_symbols()
- From: Azat Khuzhin <a3at.mail@xxxxxxxxx>
- To: Jovi Zhangwei <jovi.zhangwei@xxxxxxxxx>
- Date: Thu, 14 Nov 2013 12:46:34 +0400
On Thu, Nov 14, 2013 at 12:36 PM, Jovi Zhangwei <jovi.zhangwei@xxxxxxxxx> wrote:
> On Thu, Nov 14, 2013 at 4:27 PM, Azat Khuzhin <a3at.mail@xxxxxxxxx> wrote:
>> On Thu, Nov 14, 2013 at 6:37 AM, Jovi Zhangwei <jovi.zhangwei@xxxxxxxxx>
>> wrote:
>>> Hi Azat,
>>
>> Hi Jovi,
>>
>>>
>>> Thanks your patches.
>>>
>>> On Thu, Nov 14, 2013 at 3:13 AM, Azat Khuzhin <a3at.mail@xxxxxxxxx> wrote:
>>>> Old find_symbol() just find symbol by name, the new one
>>>> get_dso_symbols() will return all symbols/notes.
>>>>
>>>> And user of this functions, will match name/pattern by itself.
>>>>
>>>> For now get_dso_symbols() used like find_symbol(), this behaviour will
>>>> be changed in next patches.
>>>>
>>>> Signed-off-by: Azat Khuzhin <a3at.mail@xxxxxxxxx>
>>>> ---
>>>> userspace/eventdef.c | 13 ++++++-
>>>> userspace/symbol.c | 105
>>>> +++++++++++++++++++++++++++++++++++++++++----------
>>>> userspace/symbol.h | 25 +++++++++---
>>>> 3 files changed, 117 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/userspace/eventdef.c b/userspace/eventdef.c
>>>> index 748ca10..13703c6 100644
>>>> --- a/userspace/eventdef.c
>>>> +++ b/userspace/eventdef.c
>>>> @@ -333,6 +333,8 @@ static char *parse_events_resolve_symbol(char *event,
>>>> int type)
>>>> {
>>>> char *colon, *end, *tail, *binary, *symbol;
>>>> vaddr_t symbol_address;
>>>> + struct dso_symbol *symbols = NULL;
>>>> + size_t symbols_count, i;
>>>>
>>>> colon = strchr(event, ':');
>>>> if (!colon)
>>>> @@ -357,7 +359,16 @@ static char *parse_events_resolve_symbol(char *event,
>>>> int type)
>>>> symbol = strdup(colon + 1 /* skip ":" */);
>>>> }
>>>>
>>>> - symbol_address = find_symbol(binary, symbol, type);
>>>> + symbols_count = get_dso_symbols(&symbols, binary, type);
>>>> + for (i = 0; i < symbols_count; ++i) {
>>>> + if (strcmp(symbols[i].name, symbol))
>>>> + continue;
>>>> +
>>>> + symbol_address = symbols[i].addr;
>>>> + break;
>>>> + }
>>>> + free_dso_symbols(symbols, symbols_count);
>>>> +
>>>
>>> I have some concern on function get_dso_symbol, imagine one big binary
>>> have 10000+ symbols,
>>> then it's quite inefficient to allocate that symbols array, and
>>> probably not necessarily.
>>
>> Yeah, that's realloc() is freaking me out
>>
>>>
>>> How about use actor function? like below:
>>>
>>> int uprobe_symbol_actor(const char *binary, const char *symbol,
>>> unsigned long addr)
>>> {
>>> /* write to debufs */
>>> ...
>>> }
>>>
>>>
>>> nr = symbol_parse(binary, symbol, type, uprobe_symbol_actor);
>>> if (!nr) {
>>> /* no symbol found */
>>> }
>>>
>>> Then there don't need any dynamic memory allocation for symbols array.
>>> (probably this would be more proper when consider to parse function
>>> arguments
>>> and local variable in future, return a big symbols array cannot handle
>>> this feature easily)
>>
>> But I didn't want to write to debugfs from symbol module.
>> Maybe callback to get_dso_symbol() will be more appropriate?
>>
>> I was thinking about adding some "base" structure for symbols,
>> and using it allow to iterate throw symbols,
>> unfortunately this is not so simple.
>>
>> I think for now better will be implement callbacks,
>> and after we will try to find more _correct_ solution.
>>
>> What you think?
>>
> Yes, same meaning with me, the example I posted is also use callback,
> whatever the name called, actor or callback, doesn't matter.
Great :)
I just didn't understand you well.
I'll send patch on top of existing.
>
>
> Thank you.
>
> Jovi
--
Respectfully
Azat Khuzhin
Other related posts:
