[PATCH] session.cxx (systemtap_session copy ctor): copy kernel_build_tree value

[PATCH] session.cxx (systemtap_session copy ctor): copy kernel_build_tree value

[Tom Zanussi]

Hi,

Here's a patch for a problem I've seen when using systemtap to do a
'cross' invocation of a stap script on a remote target system.  I'm not
sure this is the correct fix, or if it's even proper C++, as it's been
awhile since I've done much C++, but it did fix the problem here...

When using the stap -r option with a full path to a kernel build tree
(i.e. one starting with /) along with --remote to execute the script
on a remote system, the build tree that I passed in was ignored and it
used some default locally-constructed location instead:

Checking "/lib/modules/3.4.24-yocto-standard/build/.config" failed
  with error: No such file or directory

I traced it down to the s->clone() call in the path
ssh_remote->connect()->set_child_fds(), which unconditionally invokes
the following in the systemtap_session copy constructor:

  kernel_build_tree = "/lib/modules/" + kernel_release + "/build";

That doesn't seem correct - it seems to me it should be preserving the
value from the passed-in session object, which is what this does.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 session.cxx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/session.cxx b/session.cxx
index cccade0..1410b3f 100644
--- a/session.cxx
+++ b/session.cxx
@@ -263,7 +263,7 @@ systemtap_session::systemtap_session (const systemtap_session& other,
   last_token (0)
 {
   release = kernel_release = kern;
-  kernel_build_tree = "/lib/modules/" + kernel_release + "/build";
+  kernel_build_tree = other.kernel_build_tree;
   architecture = machine = normalize_machine(arch);
   setup_kernel_release(kern.c_str());
   native_build = false; // assumed; XXX: could be computed as in check_options()
-- 
1.7.11.4

Re: [PATCH] session.cxx (systemtap_session copy ctor): copy kernel_build_tree value

[Josh Stone]

On 02/01/2013 01:32 PM, Tom Zanussi wrote:
> When using the stap -r option with a full path to a kernel build tree
> (i.e. one starting with /) along with --remote to execute the script
> on a remote system, the build tree that I passed in was ignored and it
> used some default locally-constructed location instead:
> 
> Checking "/lib/modules/3.4.24-yocto-standard/build/.config" failed
>   with error: No such file or directory
> 
> I traced it down to the s->clone() call in the path
> ssh_remote->connect()->set_child_fds(), which unconditionally invokes
> the following in the systemtap_session copy constructor:
> 
>   kernel_build_tree = "/lib/modules/" + kernel_release + "/build";
> 
> That doesn't seem correct - it seems to me it should be preserving the
> value from the passed-in session object, which is what this does.

Hmm.  Ok, I see what you're doing, and why your patch would fix it, but
that's not going to be the correct solution in general.

The reason that session ctor doesn't copy the build tree from "other" is
that we only do session clones when the arch+kernel doesn't match the
main session.  So if either arch or kernel release are different, then
we're already assuming that related fields like the build tree also must
be recomputed.

Generally, the main session represents the local environment, unless
something like -r modifies it, as you have.  I'm guessing if you were to
also specify -a to match the remote arch, it will just work on the main
session without cloning at all.

Another reason we do this "clone", instead of just modifying the main
session to match the remote, is that stap allows *multiple* remote
targets, and all of them could have different kernels.  Such mixing does
actually work fine, though it gets hairy if any of those need things
like non-default kernel build trees.  Right now I think the only way to
do this is to setup stap-server instances with the custom configurations
for each remote, then use stap --use-server with the multiple remotes.
I've thought about enhancing the remote URI parser to allow direct
remote-specific options, but so far it hasn't come up. :)

Josh

Re: [PATCH] session.cxx (systemtap_session copy ctor): copy kernel_build_tree value

[Tom Zanussi]

On Fri, 2013-02-01 at 16:54 -0800, Josh Stone wrote:
> On 02/01/2013 01:32 PM, Tom Zanussi wrote:
> > When using the stap -r option with a full path to a kernel build tree
> > (i.e. one starting with /) along with --remote to execute the script
> > on a remote system, the build tree that I passed in was ignored and it
> > used some default locally-constructed location instead:
> > 
> > Checking "/lib/modules/3.4.24-yocto-standard/build/.config" failed
> >   with error: No such file or directory
> > 
> > I traced it down to the s->clone() call in the path
> > ssh_remote->connect()->set_child_fds(), which unconditionally invokes
> > the following in the systemtap_session copy constructor:
> > 
> >   kernel_build_tree = "/lib/modules/" + kernel_release + "/build";
> > 
> > That doesn't seem correct - it seems to me it should be preserving the
> > value from the passed-in session object, which is what this does.
> 
> Hmm.  Ok, I see what you're doing, and why your patch would fix it, but
> that's not going to be the correct solution in general.
> 
> The reason that session ctor doesn't copy the build tree from "other" is
> that we only do session clones when the arch+kernel doesn't match the
> main session.  So if either arch or kernel release are different, then
> we're already assuming that related fields like the build tree also must
> be recomputed.
> 
> Generally, the main session represents the local environment, unless
> something like -r modifies it, as you have.  I'm guessing if you were to
> also specify -a to match the remote arch, it will just work on the main
> session without cloning at all.
> 

Thanks for the explanation - it ended up leading me in the right
direction.

Turns out the problem was just that I was erroneously passing 'x86-64'
instead of 'x86_64' into -a, which ultimately resulted in a clone along
with a new and, for me, bogus kernel_build_tree, etc...

Tom

> Another reason we do this "clone", instead of just modifying the main
> session to match the remote, is that stap allows *multiple* remote
> targets, and all of them could have different kernels.  Such mixing does
> actually work fine, though it gets hairy if any of those need things
> like non-default kernel build trees.  Right now I think the only way to
> do this is to setup stap-server instances with the custom configurations
> for each remote, then use stap --use-server with the multiple remotes.
> I've thought about enhancing the remote URI parser to allow direct
> remote-specific options, but so far it hasn't come up. :)
> 
> Josh