2
\$\begingroup\$

I've automated the process of extracting and converting a heap dump from an Android device/emulator.

I'd appreciate any feedback and improvements regarding style, bad practices, non-idiomatic code, performance or other issues you may find.

Also, dump_heap() function tries to wait for a daemon to complete and I'm wondering if there are better ways to handle the situation.

#!/bin/bash
#
# Extracts a heap dump from a device/emulator. 
# Assumes the appropriate tools (adb, hprof-conv) are included in the path.

SERIAL=""
PACKAGE=""
FILE=""

usage() {
    echo "Usage: -s <serialNumber> -p <package> -f <file>"
    exit 1
}

err() {
    local msg="$1"
    echo "$msg" >&2
    exit 1
}

create_file() {
    local file=$1
    local dir=$(dirname "$file")

    mkdir -p -- "$dir" && touch -- "$file"  

    if [ ! -e "$file" ]; then 
        err "File $file could not be created."
    fi
}

file_size_on_device() {
    local adbcmd="adb -s $SERIAL"
    local file="$1"
    local adbinfo="$adbcmd shell ls -l $file"

    $adbinfo | awk '{print $4}'
}

dump_heap() {
    local adbcmd="adb -s $SERIAL"
    local pid="$1"
    local file="$2"
    local dir=$(dirname "$file")

    # Remove any existing tmp files we created. Ideally, there should be none.
    local exists=$($adbcmd shell ls "$dir" | grep "$file")
    if [ -n "$exists" ]; then
        $adbcmd shell rm "$file"
    fi

    echo -n "Dumping heap "

    # Dump the heap to a tmp file on a device/emulator
    $adbcmd shell am dumpheap "$pid" "$file"

    # Beacuse the previous cmd runs as a daemon, we have to wait for it to 
    # finish. The following checks the tmp file size continuously, stopping
    # when it's not changing anymore.  
    sleepinterval=0.5

    s0=-1
    sleep $sleepinterval
    s1=$(file_size_on_device "$file")

    while ((s1 > s0))
    do 
        echo -n "."
        sleep $sleepinterval
        let s0=$s1
        let s1=$(file_size_on_device "$file")
    done

    echo " Done"
}

extract_from_device() { 
    local adbcmd="adb -s $SERIAL"

    local pid=$($adbcmd shell ps | grep $PACKAGE | awk '{print $2}')
    if [ -z "${pid}" ]; then
        err "PID for $PACKAGE not found. Is your app installed and running?"
    fi

    # Dump heap to tmp file on a device
    local file="/sdcard/hprof___tmp"
    dump_heap "$pid" "$file"

    # Extract
    local tmp="$FILE-nonconv"   
    create_file $tmp
    $adbcmd pull -p $file $tmp

    # Convert
    if [ -s "$tmp" ]; then
        create_file $FILE   
        hprof-conv $tmp $FILE
    else
        echo "No data to convert."
    fi

    # Finish
    $adbcmd shell rm "$file"
    rm -- $tmp

    echo "Done, converted heap extracted to $FILE"
}

while getopts ":s::p::f:" opt; do
    case $opt in
        s) SERIAL="$OPTARG";;
        p) PACKAGE="$OPTARG";;
        f) FILE="$OPTARG";;
        *) usage;;
    esac
done

if [ -z "${SERIAL}" ] || [ -z "${PACKAGE}" ] || [ -z "${FILE}" ]; then
    usage
else 
    version=$(adb -s "$SERIAL" shell getprop ro.build.version.sdk | tr -d '\r')
    if [ "$version" -gt "10" ]; then
        extract_from_device
    else 
        err "The target device/emulator must be API 11 or above."
    fi
fi

The above is part of Dumpey, which helps you

  • run the monkey on multiple devices or emulators and
  • make converted memory heap dumps before and after it
\$\endgroup\$
  • 1
    \$\begingroup\$ I believe that getopts string should just be :s:p:f: for silent mode and a required argument for each of the s, p and f flags. \$\endgroup\$ – Etan Reisner Feb 6 '15 at 15:41
  • 1
    \$\begingroup\$ Storing commands in strings is generally not a good idea (as with adbcmd and adbinfo). Anything complicated breaks. If you must store it using an array is much better but it doesn't look like you need to here. \$\endgroup\$ – Etan Reisner Feb 6 '15 at 15:44
  • \$\begingroup\$ Thanks, I’ll fix what you’ve pointed out. If there’s anything else fishy, please let me know. \$\endgroup\$ – Tadej Feb 6 '15 at 16:15
3
\$\begingroup\$

First of all, your code passes on http://www.shellcheck.net/# with flying colors, congrats!

Simplify variable initializations

Here you can drop all the "", it will be the same thing but easier to write:

SERIAL=""
PACKAGE=""
FILE=""

That is, like this:

SERIAL=
PACKAGE=
FILE=

Check if a regular file exists with -f instead of -e

I admire your paranoia, with all the --:

mkdir -p -- "$dir" && touch -- "$file"  

if [ ! -e "$file" ]; then 
    err "File $file could not be created."
fi

But I would also recommend to use -f instead of -e, as you are really expecting a regular file, and -e is not necessary a regular file, can be a directory too.

Avoid putting commands into variables, especially when used only once

About this code:

local adbinfo="$adbcmd shell ls -l $file"

$adbinfo | awk '{print $4}'

When the command in $adbinfo becomes anything non-trivial, running the command with $adbinfo | ... may not work as expected. As long as you keep that in mind, this is fine. However, since you only use this command once in the file_size_on_device function, I recommend to not use $adbinfo at all, but the command directly:

$adbcmd shell ls -l $file | awk '{print $4}'

Even so, just keep in mind that the same dangers exist for $adbcmd as for $adbinfo earlier. But at least we removed one layer of danger ($adbinfo), so that's a good thing.

Simplify ls | grep, use just ls when it's good enough

In dump_heap, I'm wondering if this actually works as expected:

# Remove any existing tmp files we created. Ideally, there should be none.
local exists=$($adbcmd shell ls "$dir" | grep "$file")
if [ -n "$exists" ]; then
    $adbcmd shell rm "$file"
fi

In particular, it seems to me the rm command should be like this:

$adbcmd shell rm "$dir/$file"

Because ls "$dir" | grep "$file" suggests that $file is just a filename, not an absolute path, inside the directory $dir.

Also, the call to grep seems unnecessary, a single ls command should do:

local exists=$($adbcmd shell ls "$dir/$file")

Simplifying conditions

Still in dump_heap, when checking if $exists is non-empty, instead of -n "$exists", you can just drop the -n, simply [ "$exists" ] is the same thing.

Lastly, I'm wondering if the whole thing can be simplified to:

# Remove any existing tmp files we created. Ideally, there should be none.
$adbcmd shell ls "$dir/$file" &>/dev/null && $adbcmd shell rm "$dir/$file"

Prefer printf over echo for non-trivial printing

The various flags of echo are unreliable. Depending on your system, they might work as expected or not. Rather than using any of the flags of echo, I prefer printf. So instead of:

echo -n "Dumping heap "

I recommend:

printf "Dumping heap "

However, I prefer echo when printing trivial text, without using any flags, and when I want a terminating newline.

Prefer fewer processes

In this pipeline there are 3 processes: adb, grep and awk

local pid=$($adbcmd shell ps | grep $PACKAGE | awk '{print $2}')

Especially in cases when an awk follows a grep, one begs the question if the task of grep can be included in the awk command. And in this case yes:

local pid=$($adbcmd shell ps | awk -v pkg=$PACKAGE '$0 ~ pkg {print $2}')

However, a little laziness is not against the spirit of good shell scripting. Your version with grep is shorter and simpler, and on most days I would go with it too. So this remark is just for the record, and for the meticulous.

Simplify condition

Instead of this:

if [ -z "${SERIAL}" ] || [ -z "${PACKAGE}" ] || [ -z "${FILE}" ]; then

I suggest to simplify like this:

if ! [ "$SERIAL" -a "$PACKAGE" -a "$FILE" ]; then
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, thanks for a great review janos, this is exactly what I was looking for! \$\endgroup\$ – Tadej Feb 16 '15 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.