search

iovisor / bcc

BCC - Tools for BPF-based Linux IO analysis, networking, monitoring, and more
Apache License 2.0
20.36k stars 3.86k forks source link

tools/old/profile.py: Update output format (str.decode()) for Python3 #4977

Closed bernhardkaindl closed 4 months ago

bernhardkaindl commented 5 months ago

Hi,

this fixes the output format of tools/old/profile.py for Python 3 to not have b'symbol', but just have symbol.

yonghong-song commented 4 months ago

Why you are modifying tools/old/profile.py instead of tools/profile.py? Are you actually using tools/old/profile.py in your work? Anything missing in tools/profile.py (e.g., due to old kernel)?

bernhardkaindl commented 4 months ago

Hello @yonghong-song, thanks for merging my other PR for tools/profile.py BTW!

Why you are modifying tools/old/profile.py instead of tools/profile.py?

I was using tools/old/profile.py because initially I saw that tools/old/profile.py just had this Python3 issue but worked otherwise for me. It was just the more obvious, easy fix to do.

Are you actually using tools/old/profile.py in your work?

Now that #4974 fixes the issue with the old kernel, tools/profile.py should work, so we can use it instead.

Anything missing in tools/profile.py (e.g., due to old kernel)?

As far as I can see for now, only #4974 was missing in it:

Another option would be to merge this update to Python3 just in case tools/profile.py fails for some other older kernel (now or in the future due to some other possible future change/bug).

The changes I made are only adding additional calls to .decode("utf-8", "replace") which would not make Python2 to fail, but in case comm strings contain non-ASCII chars, it could remove or replace those.

In case tools/old/profile.py has users using Python2, I think it's best to either close this PR or delay merging it to replace the .decode("utf-8", "replace") calls with a wrapper function that returns the passed string as-is on Python2 (so no change for Python2) and in Python3, returns using return arg.decode("utf-8", "replace")

I'm also open to just closing it as no longer an issue due to #4974 merged.

yonghong-song commented 4 months ago

I'm also open to just closing it as no longer an issue due to https://github.com/iovisor/bcc/pull/4974 merged.

okay, let us just close this pull request then.

bernhardkaindl commented 4 months ago

Okay, closing it as I do not need the old tool any more.