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

android-gadget-setup: fix conditional argument to assign serial variable #553

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

chirag-jn
Copy link
Contributor

@chirag-jn chirag-jn commented Nov 21, 2023

Change the conditional argument to check if androidserial is non-empty.

Initially we observed the following logs on boot-up as well as on running systemctl restart android-tools-adbd

Boot-up logs:

Apr 28 17:42:28 qcm6490 systemd[1]: Starting Android Debug Bridge...
Apr 28 17:42:28 qcm6490 systemd[1]: android-tools-adbd.service: Control process exited, code=exited, status=1/FAILURE
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[518]: /usr/bin/android-gadget-cleanup: cd: line 7: can't cd to adb: No such file or directory
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[518]: /usr/bin/android-gadget-cleanup: line 9: can't create UDC: Permission denied
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[520]: killall: adbd: no process killed
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[521]: umount: /dev/usb-ffs/adb: no mount point specified.
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[522]: rm: cannot remove 'configs/c.1/ffs.usb0': No such file or directory
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[524]: rmdir: failed to remove 'configs/c.1/strings/0x409': No such file or directory
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[525]: rmdir: failed to remove 'configs/c.1': No such file or directory
Apr 28 17:42:28 qcm6490 android-gadget-cleanup[526]: rmdir: failed to remove 'functions/ffs.usb0': No such file or directory

Logs of systemctl restart android-tools-adbd

...
Apr 28 19:04:04 qcm6490 systemd[1]: Starting Android Debug Bridge...
...
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[933]: killall: adbd: no process killed
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[934]: umount: /dev/usb-ffs/adb: not mounted.
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[936]: rm: cannot remove 'configs/c.1/ffs.usb0': No such file or directory
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[937]: rmdir: failed to remove 'configs/c.1/strings/0x409': No such file or directory
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[938]: rmdir: failed to remove 'configs/c.1': No such file or directory
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[939]: rmdir: failed to remove 'functions/ffs.usb0': No such file or dirory
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[940]: rmdir: failed to remove 'strings/0x409': No such file or directory
Apr 28 19:04:04 qcm6490 android-gadget-cleanup[941]: rmdir: failed to remove 'adb': No such file or directory
Apr 28 19:04:04 qcm6490 systemd[1]: android-tools-adbd.service: Control process exited, code=exited, status=1/FAILURE
Apr 28 19:04:04 qcm6490 systemd[1]: android-tools-adbd.service: Failed with result 'exit-code'.
...
Apr 28 19:04:04 qcm6490 systemd[1]: Stopped Android Debug Bridge.
...
(repeated 5 times)

To analyze any issue in the script, we ran $ sh -x /usr/bin/android-gadget-setup, and got the following

+ set -e
+ manufacturer=RPB
+ model='Android device'
+ serial=0123456789ABCDEF
+ '[' -r /etc/android-gadget-setup.machine ]
+ . /etc/android-gadget-setup.machine
+ manufacturer=Qualcomm
+ hostname
+ model=qcm6490
+ sed -n -e '/androidboot.serialno/  s/.*androidboot.serialno=\([^ ]*\).*/\1/gp ' /proc/cmdline
+ androidserial=
+ '[' -n  ]

Script abruptly stopped at '[' -n ], which is a syntax error. Script is running under set -e so any non-zero exit code in a command will abort the script. To fix the issue, we made the patch as provided in the commit.

After the patch, we saw the following output of $ sh -x /usr/bin/android-gadget-setup

+ set -e
+ manufacturer=RPB
+ model='Android device'
+ serial=0123456789ABCDEF
+ '[' -r /etc/android-gadget-setup.machine ]
+ . /etc/android-gadget-setup.machine
+ manufacturer=Qualcomm
+ hostname
+ model=qcm6490
+ sed -n -e '/androidboot.serialno/  s/.*androidboot.serialno=\([^ ]*\).*/\1/gp ' /proc/cmdline
+ androidserial=
+ '[' -n  ]
+ '[' -d /sys/kernel/config/usb_gadget ]
...

We can see that the script progressed past the stage of [ -n ], confirming that the patch works. Further, android-tools-adbd started working and we could see the device visible in adb devices on host machine.

Copy link
Collaborator

@lumag lumag left a comment

Choose a reason for hiding this comment

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

Commit message is pretty hard to follow. Also [ -n ] is not a syntax error, otherwise sh would have complained.

@chirag-jn
Copy link
Contributor Author

Commit message is pretty hard to follow. Also [ -n ] is not a syntax error, otherwise sh would have complained.

Updated the commit message to make it more readable. I added the logs in the PR to elaborate the issue faced.
Further, In the case of [ -n ], as visible in the output of sh -x, the output isn't coming as expected. Also, using if-else makes the code more readable and understandable.

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view.
I think just adding true or exit 0 to the end of the setup script will fix your issue.

@chirag-jn
Copy link
Contributor Author

It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view. I think just adding true or exit 0 to the end of the setup script will fix your issue.

I’ll try this and get back!

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

Also, I'm not sure how does that fix the issue for the serial number being not available in the /proc/cmdline

@chirag-jn
Copy link
Contributor Author

Also, I'm not sure how does that fix the issue for the serial number being not available in the /proc/cmdline

if serial number is not available in /proc/cmdline, then default value will be picked up. Default value is defined in /usr/bin/android-gadget-setup

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

Ok, so we should be back to #547 to get the proper serial number even if we fix this script.

@chirag-jn
Copy link
Contributor Author

Ok, so we should be back to #547 to get the proper serial number even if we fix this script.

Yeah we are working on #547 to fix reading serial number. Meanwhile, we can proceed with this PR.

@chirag-jn
Copy link
Contributor Author

It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view. I think just adding true or exit 0 to the end of the setup script will fix your issue.

Followed this. Can you check?

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view. I think just adding true or exit 0 to the end of the setup script will fix your issue.

Followed this. Can you check?

There is still a talk about the syntax error in the commit message. Also could you please check whether a single exit 0 helps? Because I think the issue is that the script returns 1 to systemd, nothing else.

@chirag-jn
Copy link
Contributor Author

exit 0 did not help. It can be tested locally too.

set 1

$ cat file1
set -e

val="someval"

if [ -r ./file2 ] ; then
        . ./file2
fi

echo $val
$ cat file2
a=""
[ -n "$a" ] && val="$a"
exit 0
$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=
++ '[' -n '' ']'
++ exit 0

set 2

$ cat file2
a=""
[ -n "$a" ] && val="$a" || true
$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=
++ '[' -n '' ']'
++ true
+ echo someval
someval

@chirag-jn
Copy link
Contributor Author

It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view. I think just adding true or exit 0 to the end of the setup script will fix your issue.

Followed this. Can you check?

There is still a talk about the syntax error in the commit message. Also could you please check whether a single exit 0 helps? Because I think the issue is that the script returns 1 to systemd, nothing else.

My mistake. Updated the commit message.

@chirag-jn
Copy link
Contributor Author

set 3

$ cat file2
a="newval"
[ -n "$a" ] && val="$a" || true
$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=newval
++ '[' -n newval ']'
++ val=newval
+ echo newval
newval

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

[ -n ] used is an incomplete conditional test. In bash and
similar shells, [ -n ] without a string following -n is considered
incomplete.

This is still not applicable to the source code in question. There is a string argument after -n, double quotes ensure that enven if the environment variable is empty, there will be an empty string argument:

[ -n "$androidserial" ] 

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

Ack, I checked, this works:

lumag@eriador:/tmp$ cat test.sh 
#!/bin/sh

set -e

if [ -r test2.sh ] ; then
	. ./test2.sh
fi

echo continued
lumag@eriador:/tmp$ cat test2.sh 
[ -n "$EMPTY" ] && echo not-empty
true
lumag@eriador:/tmp$ sh test.sh
continued
lumag@eriador:/tmp$ EMPTY=not sh test.sh
not-empty
continued

@chirag-jn
Copy link
Contributor Author

Ack, I checked, this works:

lumag@eriador:/tmp$ cat test.sh 
#!/bin/sh

set -e

if [ -r test2.sh ] ; then
	. ./test2.sh
fi

echo continued
lumag@eriador:/tmp$ cat test2.sh 
[ -n "$EMPTY" ] && echo not-empty
true
lumag@eriador:/tmp$ sh test.sh
continued
lumag@eriador:/tmp$ EMPTY=not sh test.sh
not-empty
continued

yeah exit 0 as the last command doesn't work but true as the last command works. Do I still have to make a change or the current commit works fine?

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

Yes, please.

@lumag
Copy link
Collaborator

lumag commented Nov 21, 2023

And fix the commit message too.

Change the conditional argument to check if androidserial is non-empty. The
condition `[ -n ]` when `androidserial` is empty is equated to `false` which
results in an non-zero status from `android-gadget-setup.machine`.
/usr/bin/android-gadget-setup starts with `set -e` is used to change
behavior of the shell in various ways. `-e` option causes the shell to exit as
soon a command exits with a non-zero status. Hence, executing `true` in
the end results in a zero exit status in all cases, which suffices the
condition  for `set -e`, hence unblocking android-tools-adbd service when
`androidserial` is not defined.

Signed-off-by: Chirag Jain <quic_chirjain@quicinc.com>
@chirag-jn
Copy link
Contributor Author

And fix the commit message too.

fixed the code as well as the commit message. Thanks @lumag !

@chirag-jn
Copy link
Contributor Author

@lumag so is it complete or anything else needed?

@lumag lumag merged commit 4a13096 into Linaro:master Nov 21, 2023
2 checks passed
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.

2 participants