Bug 409909 - id2ctx() does not find context if contexts hierarchy has more than 2 levels
Summary: id2ctx() does not find context if contexts hierarchy has more than 2 levels
Status: RESOLVED FIXED
Alias: None
Product: TCF
Classification: Tools
Component: Agent (show other bugs)
Version: 1.1   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 1.2   Edit
Assignee: Project Inbox CLA
QA Contact: Eugene Tarassov CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-05 03:19 EDT by Jean-Michel Pedrono CLA
Modified: 2014-05-27 21:58 EDT (History)
1 user (show)

See Also:


Attachments
allow id2ctx() to handle hierarchy with more than 2 levels (1.39 KB, patch)
2013-06-05 03:26 EDT, Jean-Michel Pedrono CLA
eugene: iplog-
Details | Diff
Allow clients to provide user defined version of id2ctx() (1.62 KB, patch)
2013-06-06 05:01 EDT, Jean-Michel Pedrono CLA
mober.at+eclipse: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Michel Pedrono CLA 2013-06-05 03:19:29 EDT
id2ctx() routine has been initially implemented for a 2 levels (Processes/Threads) hierarchy, and works fine in this case.

However, If we add one (or more) level(s) to the hierarchy, for example: System/Processes/Threads, then id2ctx() will fail.

The problem is that id2ctx() (Code pasted below for better understanding) is calling context_find_from_pid(pid, thread), with <thread> argument set to true if given id has a parent.
Calling id2ctx() with a Thread ID will work as expected, but calling id2ctx() with a Process ID won't when the process has a parent (System).

Here is current code:

Context * id2ctx(const char * id) {
    pid_t parent = 0;
    pid_t pid = id2pid(id, &parent);
    if (pid == 0) return NULL;
    return context_find_from_pid(pid, parent != 0);
}

A solution to resolve this limitation could be to no longer use <parent> value returned by id2pid(), and to call context_find_from_pid() with thread=false first, if no matching context is found then call it again with thread=true.

The other solution would be to allow clients to provide their own implementation of id2ctx().
Comment 1 Jean-Michel Pedrono CLA 2013-06-05 03:26:04 EDT
Created attachment 231970 [details]
allow id2ctx() to handle hierarchy with more than 2 levels
Comment 2 Eugene Tarassov CLA 2013-06-05 13:35:49 EDT
The code in the patch would not work: on Linux, a process and main thread in the process have same PID, so id2ctx() would always return the process object instead of the thread.

> The other solution would be to allow clients to provide their own implementation of id2ctx().

It is already the case. id2ctx() is intentionally defined in c-header file, clients are supposed to use -I compiler option to pull in a their own version of the header if necessary.

I'm changing status to "invalid".
Comment 3 Jean-Michel Pedrono CLA 2013-06-06 04:59:13 EDT
Hi Eugene,

(In reply to comment #2)
> The code in the patch would not work: on Linux, a process and main thread in
> the process have same PID, so id2ctx() would always return the process
> object instead of the thread.

Right, I missed that point.

> > The other solution would be to allow clients to provide their own implementation of id2ctx().
> 
> It is already the case. id2ctx() is intentionally defined in c-header file,
> clients are supposed to use -I compiler option to pull in a their own
> version of the header if necessary.

Right it works, but adding an #ifdef around id2ctx() would easily allow clients to provide only their own version of id2ctx() without having to duplicate pid-hash.h integrally and potentially have to handle merges of changes that could be made to the original version.
 
> I'm changing status to "invalid".

Could you please have a look the 2nd patch I'm submitting?

Thanks.
Comment 4 Jean-Michel Pedrono CLA 2013-06-06 05:01:12 EDT
Created attachment 232031 [details]
Allow clients to provide user defined version of id2ctx()
Comment 5 Eugene Tarassov CLA 2013-06-07 11:49:44 EDT
I have committed the patch.
Thanks!
Comment 7 Martin Oberhuber CLA 2014-05-27 21:58:25 EDT
Comment on attachment 232031 [details]
Allow clients to provide user defined version of id2ctx()

Marking iplog- since Jean-Michel is correctly tracked as Author in GIT.