[ktap] Re: [PATCH 1/3] symbol: replace find_symbol() by get_dso_symbols()
- From: Jovi Zhangwei <jovi.zhangwei@xxxxxxxxx>
- To: Azat Khuzhin <a3at.mail@xxxxxxxxx>
- Date: Thu, 14 Nov 2013 16:36:55 +0800
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.
Thank you.
Jovi
Other related posts:
