usdt: fail when binary doesn't exist. Fixes #1749 #1762
Conversation
javierhonduco
requested review from
4ast,
drzaeus77,
goldshtn and
yonghong-song
as code owners
|
[buildbot, ok to test] |
src/cc/usdt/usdt.cc
Outdated
| else | ||
| } else { | ||
| struct stat buffer; | ||
| if ((strlen(path) >= 1 && path[0] != '/' ) || stat(path, &buffer) == -1) { |
There was a problem hiding this comment.
Could you put the error messages here? You can see from libbpf.c and usdt.cc where a lot of errors are printed out simply with fprintf(stderr, ...). This way, in python, you do not need to check startwith('/' or open(path) any more, you can just throw an exception since the detailed errors and hints have been printed out.
If you added the error messages in bcc_usdt_new_frompid, C++ and lua will be automatically covered.
There was a problem hiding this comment.
Makes sense! I was somehow trying to avoid printing from there for some reason. Should have checked before if that was done in these files :)
src/python/bcc/usdt.py
Outdated
| @@ -129,10 +129,28 @@ def __init__(self, pid=None, path=None): | |||
| self.pid = pid | |||
| if path: | |||
| self.context = lib.bcc_usdt_new_frompid(pid, path.encode('ascii')) | |||
| if self.context == None: | |||
| if not path.startswith('/'): | |||
There was a problem hiding this comment.
The whole if ... else ... can be done in usdt.cc.
javierhonduco
force-pushed
the
usdt-inexistent-path
branch
2 times, most recently
from
e851ce6
to
92b436d
Compare
And add error message to hint if the problem is that the passed binary path is not absolute or if the binary doesn't exist. In case the PID is correct: * but the binary couldn't be found, it will print: ``` HINT: Specified binary doesn't exist. [...] ``` * but the binary is not absolute: ``` HINT: Binary path should be absolute. [...] ``` Otherwise, it should keep behaving as before.
javierhonduco
force-pushed
the
usdt-inexistent-path
branch
from
92b436d
to
dcb9b9a
Compare
|
@yonghong-song I think it's ready for a second review 馃槃 BTW, didn't add the checks to |
|
LGTM. Thanks! |
And add error message to hint if the problem is that the passed binary
path is not absolute or if the binary doesn't exist.
In case the PID is correct:
but the binary couldn't be found, it will print:
but the binary is not absolute:
Otherwise, it should keep behaving as before.
Let me know if there are better approaches to do this (first steps in C++ here 馃槃)!