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.

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

Display 30 chars for process name #63

Merged
merged 1 commit into from
Jan 4, 2020
Merged

Conversation

zhangxp1998
Copy link
Collaborator

bandwhich uses lsof to obtain process names. On MacOS, lsof defaults to only showing first 9 characters of process names.

Xnip2020-01-02_12-30-50

This is slightly confusing sometimes, for example, in the above screenshot, what is "Google"? I think we should tweek lsof to display more characters. (30 in this commit)

After:

Xnip2020-01-02_12-31-25

@imsnif
Copy link
Owner

imsnif commented Jan 3, 2020

I like this! I'll give it a full review a little later, but for now - do you think we can get rid of the extra chars somehow (eg. \x20)? I'm guessing this is some encoding issue

@zhangxp1998
Copy link
Collaborator Author

Re \x20: Looks like lsof automatically escape space characters into “\x20”. We can “unescape” it, but there might be other characters that lsof is escaping.

@Alcaro
Copy link

Alcaro commented Jan 3, 2020

lsof escapes anything for which iswprint() returns false, as well as backslash.

It supports a large variety of escaping schemes - may be annoying to undo. Especially since both \x03 and ^C are printed as ^C.

(^? will never be printed on x86, since char is signed, so (unsigned)(char)-1 is 0xFFFFFFFF. But it will show up on ARM. Fun.)

@zhangxp1998
Copy link
Collaborator Author

lsof escapes anything for which iswprint() returns false, as well as backslash.

It supports a large variety of escaping schemes - may be annoying to undo. Especially since both \x03 and ^C are printed as ^C.

(^? will never be printed on x86, since char is signed, so (unsigned)(char)-1 is 0xFFFFFFFF. But it will show up on ARM. Fun.)

Strange that it treats the space character as non printable. Maybe we can simply replace all \x20 with space characters? Other escaped chars are probably non-printable, we should leave them alone.

@zhangxp1998 zhangxp1998 force-pushed the procname branch 2 times, most recently from c1b54bb to ff16687 Compare January 3, 2020 19:40
@zhangxp1998
Copy link
Collaborator Author

Now it looks like:
Xnip2020-01-03_14-38-29

@Alcaro
Copy link

Alcaro commented Jan 3, 2020

Oh, you're right. Space is indeed printable per isprint(); I misread the code. Space is another special case, alongside backslash.

I agree with leaving escaped unprintables as is, and unescaping space. Backslash should probably be unescaped too, to make Wine programs look better (though lsof seems to not print their paths?).

@@ -62,7 +62,7 @@ impl RawConnection {
}

pub fn get_connections<'a>() -> RawConnections {
let content = run(&["-n", "-P", "-i4"]);
let content = run(&["-n", "-P", "-i4", "+c", "30"]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I do lsof -n -P -i4 +c 30 on my machine, I get lsof: +c 30 > what system provides (15)
I looked through the lsof man page and under the +c w section, I found this: "If w is zero ('0'), all command characters supplied to lsof by the UNIX dialect will be printed"

What do you think about changing "30" to "0"? That way we just get the max possible value on each machine?

Copy link
Collaborator Author

@zhangxp1998 zhangxp1998 Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our UI will correctly truncate the string to size of user's terminal, sure. (What if the process name is too long that it overflows display)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm looks like most processes on my machine have names < 15 chars, I don't think that will be a problem for many people. Let me update the code

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - we should truncate them in the UI. I think it already does that with the connection strings, which are horribly long. :)

@imsnif
Copy link
Owner

imsnif commented Jan 4, 2020

LGTM - thanks for another great PR @zhangxp1998 :)

@imsnif imsnif merged commit 4f33b9a into imsnif:master Jan 4, 2020
@zhangxp1998 zhangxp1998 deleted the procname branch January 4, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants