Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix string type handling in attach_kprobe() #623

Closed
markdrayton opened this issue Jul 22, 2016 · 1 comment
Closed

Fix string type handling in attach_kprobe() #623

markdrayton opened this issue Jul 22, 2016 · 1 comment

Comments

@markdrayton
Copy link
Contributor

As discovered in #615, attach_kprobe() with from __future__ import unicode_literals makes an assertion failure:

# ./cachetop.py
Traceback (most recent call last):
  File "./cachetop.py", line 255, in <module>
    curses.wrapper(handle_loop, args)
  File "/usr/lib/python2.7/curses/wrapper.py", line 43, in wrapper
    return func(stdscr, *args, **kwds)
  File "./cachetop.py", line 177, in handle_loop
    b.attach_kprobe(event="add_to_page_cache_lru", fn_name="do_count")
  File "/usr/lib/python2.7/dist-packages/bcc/__init__.py", line 362, in attach_kprobe
    assert isinstance(event, str), "event must be a string"
AssertionError: event must be a string

The assert exists because we later use the key type to determine how to unregister a kprobe:

bcc/src/python/bcc/__init__.py

Line 770 in f4bf275

if isinstance(k, str):

We can probably make this more robust by splitting the probe types or filtering in a way that matches all string types.
@markdrayton markdrayton mentioned this issue Jul 22, 2016
Merged
chantra added a commit to chantra/bcc that referenced this issue Jul 23, 2016
@chantra
[cachetop] fix and doc
5c6516b 
* pass -fno-color-diagnostics to clang
* remove unicode import (iovisor#623)
* add time to cachetop output
* add keybindings to cachetop.8
* add cachetop links to README.md
chantra added a commit to chantra/bcc that referenced this issue Jul 23, 2016
@chantra
[cachetop] fix and doc
e159f7e 
* pass -fno-color-diagnostics to clang
* remove unicode import (iovisor#623)
* add time to cachetop output
* add keybindings to cachetop.8
* add cachetop links to README.md
markdrayton referenced this issue Jul 27, 2016
Store kprobes with string keys, fix num_open_kprobes
8fa8c0e 
Prior to this diff we used inconsistent types for keys in `open_kprobes`. The
results from the regex match (`attach_kprobe(event_re=..)`) and the automatic
`kprobe__` features were passed through `str.decode()`, yielding unicode keys,
but specific matches (i.e. from `attach_kprobe(event=..)`) were stored with
string keys passed down from the caller. Only probes under string keys were
released in `cleanup_kprobes`, leaving attached probes on program exit.

This diff makes all the keys regular strings. I erred on the side of using
regular strings over `str.decode()`ing them because a) this data isn't passed
outside of Python, b) it's more Python 3 compatible (there is no `.decode()` on
a regular string object in Python 3 so such a change would ultimately need
removing again).

I also cleaned up a few other things:

* removed the call to `awk` for getting probable functions

* removed the `isinstance` checks when cleaning uprobes/tracepoints -- we
  should only have string keys in these dicts

* made `num_open_kprobes` skip the perf_events buffers. People likely use this
  to check that the right number of probes have been placed so counting
  perf_events buffers doesn't make sense here
@markdrayton markdrayton mentioned this issue Jul 27, 2016
Merged
@markdrayton
Copy link
Contributor Author

closed via #632.

palmtenor pushed a commit to palmtenor/bcc that referenced this issue Jul 31, 2016
@chantra @palmtenor
[cachetop] fix and doc
5f41002 
* pass -fno-color-diagnostics to clang
* remove unicode import (iovisor#623)
* add time to cachetop output
* add keybindings to cachetop.8
* add cachetop links to README.md
palmtenor pushed a commit to palmtenor/bcc that referenced this issue Jul 31, 2016
@palmtenor
Remove asserts on str probe names
20fb2ba 
`open_kprobes` is a dict of open kprobes. Its keys are strings for normal
probes and a tuple for perf buffers. Normal probes need unregistering on script
exit; perf buffers do not. `cleanup` currently looks for string keys
(specifically type `str`) when working out what to unregister, which is a bit
brittle -- in Python2 strings can be both native `str` and `unicode`, depending
what exactly was passed to `attach-*/detach_*` and whether `from __future__
import unicode_literals` is used (e.g. iovisor#623).

This diff makes the API more relaxed by casting the probe name to `str` to
match the expectations of `cleanup`. This works in py2 (with and without
unicode_literals) and py3.
chantra pushed a commit to chantra/bcc that referenced this issue Sep 16, 2016
@chantra
Remove asserts on str probe names
4668532 
`open_kprobes` is a dict of open kprobes. Its keys are strings for normal
probes and a tuple for perf buffers. Normal probes need unregistering on script
exit; perf buffers do not. `cleanup` currently looks for string keys
(specifically type `str`) when working out what to unregister, which is a bit
brittle -- in Python2 strings can be both native `str` and `unicode`, depending
what exactly was passed to `attach-*/detach_*` and whether `from __future__
import unicode_literals` is used (e.g. iovisor#623).

This diff makes the API more relaxed by casting the probe name to `str` to
match the expectations of `cleanup`. This works in py2 (with and without
unicode_literals) and py3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@markdrayton