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: #1690 unable to identify shebang when it has initial spaces #1697

Merged
merged 1 commit into from
May 12, 2023
Merged

fix: #1690 unable to identify shebang when it has initial spaces #1697

merged 1 commit into from
May 12, 2023

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented May 10, 2023 •
edited
Loading

fix: #1690
Signed-off-by: Yi Hong [email protected]

namhyung
namhyung reviewed May 11, 2023
View reviewed changes
utils/utils.h Outdated
@@ -327,6 +327,31 @@ static inline char *has_kernel_filter(char *buf)
return NULL;
}

// this code is copy from event-utils.h
static inline char *str_trim(char *string)
Copy link
Owner

Choose a reason for hiding this comment

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

Please make it not inlined, and move it to utils.c file. Also I prefer a short name like 'str' or just 's' instead of 'string'.

utils/utils.h Outdated
break;
string++;
}
ret = string;
Copy link
Owner

Choose a reason for hiding this comment

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

Actually the trim function can have two separate siblings like 'ltrim' and 'rtrim'. The above is ltrim which works for beginning of the input string (the left part). While the rtrim would work for the end of the input string (the right part).

What we actually need here is 'ltrim'. So please add both ltrim and rtrim and use ltrim here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy that, thanks

cmds/record.c Outdated
@@ -1608,7 +1608,8 @@ static void check_binary(struct uftrace_opts *opts)
pr_err("Cannot read '%s'", opts->exename);

if (memcmp(elf_ident, ELFMAG, SELFMAG)) {
char *script = check_script_file(opts->exename);
// for issue 1690 unable to identify shebang when it has initial spaces
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer C-style comments (with /* ... */) over C++ (with // ...). But I think we can just get rid of it.

@yihong0618 yihong0618 requested a review from namhyung May 11, 2023 08:06
@yihong0618
Copy link
Contributor Author

@namhyung fixed thank you very much for your kind review

@honggyukim
Copy link
Collaborator

honggyukim commented May 11, 2023 •
edited
Loading

Hi @yihong0618, thanks for working on this.

From 667ffe14ed554585a613291f2ba9b9de9fc67474 Mon Sep 17 00:00:00 2001
From: yihong0618 <[email protected]>
Date: Wed, 10 May 2023 22:08:21 +0800
Subject: [PATCH] record: skip whitespaces after shebang for scripts

Python tracing won't work when the shebang line has a space like below:

Fixed: #1690

Signed-off-by: yihong0618 <[email protected]>

Could you please write your real name in your git author name instead of yihong0618?

@yihong0618 yihong0618 force-pushed the master branch 2 times, most recently from 813d6f7 to d9c9031 Compare May 11, 2023 08:11
@yihong0618
Copy link
Contributor Author

yihong0618 commented May 11, 2023 •
edited
Loading

@honggyukim

sorry I changed it but DCO failed

@honggyukim
Copy link
Collaborator

honggyukim commented May 11, 2023 •
edited
Loading

Signed-off-by: yihong [email protected]

Maybe can I ask you to do this way? For example, I assume your name is "Yi Hong". (sorry if not)

$ git config --global user.name "Yi Hong"

$ git commit -s --amend --author "Yi Hong <[email protected]>"

--author option is to change the git author name. It's not changed even if you modified the name in Signed-off-by.

@yihong0618
Copy link
Contributor Author

yihong0618 commented May 11, 2023 •
edited
Loading

@honggyukim thanks for that, really learned a lot.

@honggyukim
Copy link
Collaborator

honggyukim commented May 11, 2023 •
edited
Loading

It looks good now.

From 42dc0dd329fb881c884b8ec8f907a5dcc1ca5a6f Mon Sep 17 00:00:00 2001
From: Yi Hong <[email protected]>
Date: Wed, 10 May 2023 22:08:21 +0800
Subject: [PATCH] record: skip whitespaces after shebang for scripts

Python tracing won't work when the shebang line has a space like below:

Fixed: #1690
Signed-off-by: Yi Hong <[email protected]>

I'm sorry for asking another change again, but your description mentions

when the shebang line has a space like below:

But it doesn't show the info at the BELOW. Could you rewrite it this way?

Subject: [PATCH] record: skip whitespaces after shebang for scripts

Python tracing won't work when the shebang line has a space like below:

  #! /usr/bin/env python3

This patch makes uftrace to understand the above shebang as well.

Fixed: #1690
Signed-off-by: Yi Hong <[email protected]>

If you don't want to update again, then it's okay to skip it.

@yihong0618
Copy link
Contributor Author

It looks good now.

From 42dc0dd329fb881c884b8ec8f907a5dcc1ca5a6f Mon Sep 17 00:00:00 2001
From: Yi Hong <[email protected]>
Date: Wed, 10 May 2023 22:08:21 +0800
Subject: [PATCH] record: skip whitespaces after shebang for scripts

Python tracing won't work when the shebang line has a space like below:

Fixed: #1690
Signed-off-by: Yi Hong <[email protected]>

I'm sorry for asking another change again, but your description mentions

when the shebang line has a space like below:

But it doesn't show the info at the BELOW. Could you rewrite it this way?

Subject: [PATCH] record: skip whitespaces after shebang for scripts

Python tracing won't work when the shebang line has a space like below:

  #! /usr/bin/env python3

This patch makes uftrace to understand the above shebang as well.

Fixed: #1690
Signed-off-by: Yi Hong <[email protected]>

If you don't want to update again, then it's okay to skip it.

will do this now.

@yihong0618
record: skip whitespaces after shebang for scripts
3b4d928 
Python tracing won't work when the shebang line has a space like below:

  #! /usr/bin/env python3

This patch makes uftrace to understand the above shebang as well.

Fixed: namhyung#1690
Signed-off-by: Yi Hong <[email protected]>
@yihong0618
Copy link
Contributor Author

@honggyukim Done and thanks again

honggyukim
honggyukim approved these changes May 11, 2023
View reviewed changes
Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your work!

namhyung
namhyung approved these changes May 12, 2023
View reviewed changes
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

LGTM

@namhyung namhyung merged commit 6670ea5 into namhyung:master May 12, 2023
@honggyukim
Copy link
Collaborator

Hmm.. Shouldn't we handle the trailing whitespaces?

We may need another str_rtrim or create another wrapper str_trim for both left and right trim.

@GabrielKimm
Copy link

I'll try it.

@GabrielKimm GabrielKimm mentioned this pull request Aug 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namhyung namhyung namhyung approved these changes

@honggyukim honggyukim honggyukim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unable to identify shebang when it has initial spaces
4 participants
@yihong0618 @honggyukim @GabrielKimm @namhyung