Closed
Bug 906816
Opened 11 years ago
Closed 11 years ago
Modify run-gdb.sh to attach by process name
Categories
(Firefox OS Graveyard :: GonkIntegration, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 2 obsolete files)
1.34 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
A bit annoyed at having to run 'adb shell b2g-ps', find pid, run-gdb.sh attach so I patched run-gdb.sh to do what I want.
Attachment #792345 -
Flags: review?(justin.lebar+bug)
Comment 1•11 years ago
|
||
Comment on attachment 792345 [details] [diff] [review] patch Review of attachment 792345 [details] [diff] [review]: ----------------------------------------------------------------- ::: run-gdb.sh @@ +18,4 @@ > if [ "$1" = "attach" -a -n "$2" ] ; then > B2G_PID=$2 > if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then > + B2G_PID=$(adb shell b2g-ps | sed -e "1d" | grep "^$2" | awk '{print $3}') I'm concerned about needing sed/grep and awk and having this work on all platforms. I think it would be better to add an option to b2g-info to take a process name and return a pid, and call that instead.
Comment 3•11 years ago
|
||
Comment on attachment 792345 [details] [diff] [review] patch Dave is a better person to review this. I'm not sure b2g-info is on all phones by now, though. For example, on Leo, we can only flash gecko and gaia, so you have to push b2g-info yourself.
Attachment #792345 -
Flags: review?(justin.lebar+bug) → review?(dhylands)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #1) > Comment on attachment 792345 [details] [diff] [review] > patch > > Review of attachment 792345 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: run-gdb.sh > @@ +18,4 @@ > > if [ "$1" = "attach" -a -n "$2" ] ; then > > B2G_PID=$2 > > if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then > > + B2G_PID=$(adb shell b2g-ps | sed -e "1d" | grep "^$2" | awk '{print $3}') > > I'm concerned about needing sed/grep and awk and having this work on all > platforms. > > I think it would be better to add an option to b2g-info to take a process > name and return a pid, and call that instead. It's a developer script so worse case these users are no worse off. I'm looking into patching b2g-info.
Comment 5•11 years ago
|
||
So, actually, toolbox ps NAME will already do the "find a process by name" portion. This combines b2g-ps and the grep portion. Then you could do: B2G_PID=$(echo $(adb shell 'toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n $pid)')) and then you don't need sed, grep or awk.
Comment 6•11 years ago
|
||
Actually the $(echo ) is unecessary. so this simplifies to: B2G_PID=$(adb shell 'toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n $pid)')
Comment 7•11 years ago
|
||
All right - I think that this is my final answer :) To allow for the process name to contain embedded spaces we need to surround $B2G_PID in double quotes and pass down single quotes to toolbox ps so that it gets a single arg (I tested using 'FM Radio') B2G_PID=$(adb shell 'toolbox ps '"'$B2G_PID'"' | (read header; read user pid rest; echo -n $pid)') If you want to talk on IRC about how shell quoting works (its a little different from C++) then let me know. I tested this by creating a small script I called find-app-name.sh which contained: #!/bin/bash #set -x B2G_PID=$1 B2G_PID=$(adb shell 'toolbox ps '"'$B2G_PID'"' | (read header; read user pid rest; echo -n $pid)') echo B2G_PID=${B2G_PID} And then ran: ./find-app-name.sh 'FM Radio' (after launching FM Radio on the phone.
Comment 8•11 years ago
|
||
An alternative way of quoting this would be: B2G_PID=$(adb shell "toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n \$pid)")
Comment 9•11 years ago
|
||
And after all that, I see that run-gdb.sh already has this pattern when trying to figure out the pid of gdbserver and another case where it figures out the pid of b2g. So we should write a function (which would go near the top of the file): get_pid_by_name() { echo $(adb shell "toolbox ps '$1' | (read header; read user pid rest; echo -n \$pid)") } and then around line 15: GDBSERVER_PID=$(get_pid_by_name gdbserver) And around line 28: B2G_PID=$(get_pid_by_name b2g) and then for the attach case, do: B2G_PID=$(get_pid_by_name "$B2G_PID") The double quotes are needed in the last case since B2G_PID may contain embedded spaces.
Assignee | ||
Comment 10•11 years ago
|
||
Ahh yes, toolbox is already in the script so let's start using that. I replace adb with $ADB to conform with the script. I also added a line: 'Found Camera PID: 762'. It's a bit redundant since will print the pid it's attaching to but I find it handy.
Assignee: nobody → bgirard
Attachment #792345 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #792345 -
Flags: review?(dhylands)
Attachment #792425 -
Flags: review?(dhylands)
Comment 11•11 years ago
|
||
Comment on attachment 792425 [details] [diff] [review] patch Review of attachment 792425 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed ::: run-gdb.sh @@ +1,5 @@ > #!/bin/bash > #set -x > > +get_pid_by_name() { > + echo $($ADB shell "toolbox ps '$1' | (read header; read user pid rest; echo -n \$pid)") nit: trailing space @@ +20,4 @@ > > GDB_PORT=$((10000 + $(id -u) % 50000)) > if [ "$1" = "attach" -a -n "$2" ] ; then > + ATTACH_TARGET=$2 nit: I would move this assignment inside the if below, since ATTACH_TARGET is only needed inside that if, and I'd probably change it to be; ATTACH_TARGET=$B2G_PID (rather than $2) @@ +24,4 @@ > B2G_PID=$2 > if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then > + B2G_PID=$(get_pid_by_name "$B2G_PID") > + if [ -z $B2G_PID ] ; then nit: It's common to surround vars in double quotes when using -z or -n (so that you pass an empty string rather than no arguments) I see my current version of bash works either way, but things break down if you later need to combine with other operators.
Attachment #792425 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 12•11 years ago
|
||
What's the landing procedure for this repo?
Attachment #792425 -
Attachment is obsolete: true
Attachment #792446 -
Flags: review+
Comment 13•11 years ago
|
||
Send a PR and I (or, probably, Dave) can merge it.
Assignee | ||
Comment 14•11 years ago
|
||
https://github.com/mozilla-b2g/B2G/pull/267
Comment 15•11 years ago
|
||
https://github.com/mozilla-b2g/B2G/commit/dcd6e13d2f119444624db62d0962715bc282e090
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•