check adb runnable before starting scrcpy #236

npes87184 · 2018-08-21T13:34:33Z

There are many user who encounters missing adb.
To stop things happens again, we check it and show
sexy response to user.

In the first version, I only check in linux. If you like this idea, I will continue to finish windows part.
Thanks.

Signed-off-by: yuchenlin [email protected]

rom1v · 2018-08-21T14:02:34Z

There are many user who encounters missing adb.
To stop things happens again, we check it and show sexy response to user.

I agree, this is a good idea to show a better error message than:

ERROR: "adb push" returned with value 1

However, I think the detection should not rely on the existence of the binary (iterating over PATH directories): it's just an heuristic which will probably fail in some corner cases.

Instead, we can report this problem properly if exec() fails + errno (which is distinguishable from the command exited with a signal or a non-zero value).

In this case, cmd_execute() would need the parent process to wait for the child process until it has exec() (but not until it has terminated, the server is long-lived). For example using pipe2() with flag O_CLOEXEC and make the child write something if exec fails (and auto-close otherwise), and the parent executing a blocking read().

Then it would report this information back to the caller in some way (its signature should be changed).

npes87184 · 2018-08-21T14:35:44Z

Hi, @rom1v

I agree that it's just an heuristic method. However, I think the execvp is done by this method and I found the implementation of execvp which proves my assertion. The original idea is to find how does exec do and extract the part we need. If you don't like this method, I will re-write the code base on your advice. Thank you.

Sincerely.

rom1v · 2018-08-21T14:39:44Z

However, I think the execvp is done by this method and I found the implementation of execvp which proves my assertion.

Then, this is a good reason to use the result of exec() instead of reimplementing it :)

npes87184 · 2018-08-21T14:46:13Z

Thank you for your advice. I will write a prototype these days.

npes87184 · 2018-09-01T02:25:46Z

Hi, @rom1v

I have modified the commit base on your advice.
In linux, the cmd_execute() will return child exec state (0: success, -1: cmd_execute fail, otherwise: errno) now.
We can check child state in adb_execute() and show some sexy response to user.

The result will look like:

ERROR: Missing adb. You need adb, accessible from your PATH to execute scrcpy.
ERROR: Could not execute "adb push"

Thanks.

rom1v

Great job 👍

I added my remarks inline, mainly:

finally replace pipe2() by pipe()+fcntl() because it's not portable
no need to change the signature of cmd_execute() if the error is not reported to the caller of adb_execute()

rom1v · 2018-09-02T13:02:43Z

app/src/command.c

-    return cmd_execute(cmd[0], cmd);
+    r = cmd_execute(cmd[0], cmd, &process);
+    if (r != 0) {
+        show_err_msg(r);


If the error is not reported to the caller of adb_execute() (I'm ok with this), the relevant error message can be printed directly in the specific implementation.

In my opinion, the cmd_exectue is a general function. That is, it can execute any cmd. When it errors, problem should be handled by caller or propagate to the caller's caller. In this case, caller adb_execute handles the problem and stop propagating the failure. The error is handeled by showing error msg in adb_execute.

Oh, yes, of course. I confused adb_execute() and cmd_execute() (initially, I thought that even adb_execute() would report error to the caller). Whatever. It's better like that.

rom1v · 2018-09-02T13:03:27Z

app/src/command.h

@@ -32,7 +32,7 @@
 #endif
 # define NO_EXIT_CODE -1

-process_t cmd_execute(const char *path, const char *const argv[]);
+int cmd_execute(const char *path, const char *const argv[], process_t *process);


(and if the error message is printed directly in the specific implementation, there is no need to change this signature: PROCESS_NONE will report the error)

rom1v · 2018-09-02T13:05:09Z

app/src/sys/unix/command.c

@@ -1,24 +1,66 @@
+#define _GNU_SOURCE


Oh, I didn't realize that pipe2() required this. This would make scrcpy non portable (it would not compile on some platforms, possibly including Mac OS X (?)).

So we need pipe()+fcntl() instead (cf below).

rom1v · 2018-09-02T13:38:22Z

app/src/sys/unix/command.c

+    } else if (*pid == 0) {
+        /* child close read side. */
+        close(fd[0]);
+        fd[0] = -1;


not necessary, you exec or _exit() before the end of the block.

Close all fds before exiting is a good habit?

close(fd[0]), yes.
fd[0] = -1 is not necessary.

rom1v · 2018-09-02T13:39:21Z

app/src/sys/unix/command.c

-    return pid;
+
+END:
+    if (fd[0] != -1) {


may never happen

There is a possible path will need it, that is if we create pipe successfully but fork failed.

rom1v · 2018-09-02T13:40:08Z

app/src/sys/unix/command.c

+        close(fd[0]);
+    }
+    if (fd[1] != -1) {
+        close(fd[1]);


can be moved to the parent block

There is a possible path will need it, that is if we create pipe successfully but fork failed. I remain it in this position.

rom1v · 2018-09-02T13:41:10Z

app/src/sys/unix/command.c

+    } else if (*pid == 0) {
+        /* child close read side. */
+        close(fd[0]);
+        fd[0] = -1;


To enable CLOEXEC without pipe2(), add:

if (fcntl(fd[1], F_SETFD, FD_CLOEXEC) == -1) { perror("fcntl"); }

rom1v · 2018-09-02T13:43:13Z

app/src/sys/unix/command.c

+        fd[1] = -1;
+        /* and blocking read until child exec or write some fail
+         * reason in fd. */
+        if (0 > read(fd[0], &ret, sizeof(int))) {


(please reverse the operands)

rom1v · 2018-09-02T13:44:24Z

app/src/sys/win/command.c

@@ -4,7 +4,7 @@
 #include "log.h"
 #include "str_util.h"

-HANDLE cmd_execute(const char *path, const char *const argv[]) {
+int cmd_execute(const char *path, const char *const argv[], HANDLE *handle) {


(same here)

There are many user who encounters missing adb. To stop things happens again, we check it and show sexy response to user. Signed-off-by: yuchenlin <[email protected]>

npes87184 · 2018-09-03T12:19:36Z

Hi, @rom1v

I have modified commit based on our discussion.

v2 -> v3

pipe()+fcntl() to maintain portability
reverse the operands
only include errno.h in unix-like system.

Thanks.

rom1v

Great job! 👍 Thank you.

I commented inline, mostly nitpicking/codestyle.

The only important things are:

replace -1 by PROCESS_NONE (for Windows)
send error on fcntl() failure

One remaining thing: could we detect missing adb on Windows like on *nux?

Please find my suggestions (as a diff) below. I took the liberty to rewrite some comments and error messages. Please tell if you disagree with them.

diff --git a/app/src/command.c b/app/src/command.c
index 4f3e8d4..10e5ef9 100644
--- a/app/src/command.c
+++ b/app/src/command.c
@@ -1,8 +1,7 @@
 #include "command.h"
 
-#ifdef __WINDOWS__
-#else
-#include <errno.h>
+#ifndef __WINDOWS__
+# include <errno.h>
 #endif
 #include <stdio.h>
 #include <stdlib.h>
@@ -24,27 +23,27 @@ static inline const char *get_adb_command() {
 
 static void show_err_msg(int err) {
 #ifdef __WINDOWS__
-    LOGE("Failed to execute adb.\n");
+    (void) err; // unused
+    LOGE("Failed to execute adb");
 #else
     switch (err) {
-    case 0:
-        break;
-    case -1:
-        LOGE("Failed to execute adb.\n");
-        break;
-    case ENOENT:
-        LOGE("Missing adb. You need adb, accessible from your PATH to execute scrcpy.\n");
-        break;
-    default:
-        LOGE("Failed to execute adb, errno: [%d].\n", err);
-        break;
+        case -1:
+            LOGE("Failed to execute adb");
+            break;
+        case ENOENT:
+            LOGE("'adb' command not found (make it accessible from your PATH "
+                  "or define its full path in the ADB environment variable)");
+            break;
+        default:
+            LOGE("Failed to execute adb: %s", strerror(err));
+            break;
     }
 #endif
 }
 
 process_t adb_execute(const char *serial, const char *const adb_cmd[], int len) {
     const char *cmd[len + 4];
-    int i, r;
+    int i;
     process_t process;
     cmd[0] = get_adb_command();
     if (serial) {
@@ -57,10 +56,10 @@ process_t adb_execute(const char *serial, const char *const adb_cmd[], int len)
 
     memcpy(&cmd[i], adb_cmd, len * sizeof(const char *));
     cmd[len + i] = NULL;
-    r = cmd_execute(cmd[0], cmd, &process);
+    int r = cmd_execute(cmd[0], cmd, &process);
     if (r != 0) {
         show_err_msg(r);
-        return -1;
+        return PROCESS_NONE;
     }
     return process;
 }
diff --git a/app/src/sys/unix/command.c b/app/src/sys/unix/command.c
index 53d0bb1..20218cb 100644
--- a/app/src/sys/unix/command.c
+++ b/app/src/sys/unix/command.c
@@ -13,7 +13,7 @@ int cmd_execute(const char *path, const char *const argv[], pid_t *pid) {
     int fd[2];
     int ret = 0;
 
-    if (0 > pipe(fd)) {
+    if (pipe(fd) == -1) {
         perror("pipe");
         return -1;
     }
@@ -22,42 +22,38 @@ int cmd_execute(const char *path, const char *const argv[], pid_t *pid) {
     if (*pid == -1) {
         perror("fork");
         ret = -1;
-        goto END;
+        goto end;
     }
 
     if (*pid > 0) {
-        /* parent close write side. */
+        // parent close write side
         close(fd[1]);
         fd[1] = -1;
-        /* and blocking read until child exec or write some fail
-         * reason in fd. */
+        // wait for EOF or receive errno from child
         if (read(fd[0], &ret, sizeof(int)) == -1) {
             perror("read");
             ret = -1;
-            goto END;
+            goto end;
         }
     } else if (*pid == 0) {
-        /* child close read side. */
+        // child close read side
         close(fd[0]);
-        if (fcntl(fd[1], F_SETFD, FD_CLOEXEC) == -1) {
+        if (fcntl(fd[1], F_SETFD, FD_CLOEXEC) == 0) {
+            execvp(path, (char *const *)argv);
+        } else {
             perror("fcntl");
-	    /* To prevent parent hang in read, we should exit when error. */
-            close(fd[1]);
-	    _exit(1);
         }
-        execvp(path, (char *const *)argv);
-        /* if child execvp failed, before exit, we should write
-         * reason into the pipe. */
+        // send errno to the parent
         ret = errno;
         if (write(fd[1], &ret, sizeof(int)) == -1) {
             perror("write");
         }
-        /* close write side before exiting. */
+        // close write side before exiting
         close(fd[1]);
         _exit(1);
     }
 
-END:
+end:
     if (fd[0] != -1) {
         close(fd[0]);
     }

rom1v · 2018-09-03T19:23:19Z

app/src/command.c

@@ -1,5 +1,9 @@
 #include "command.h"

+#ifdef __WINDOWS__
+#else


#ifndef __WINDOWS__

rom1v · 2018-09-03T19:23:28Z

app/src/command.c

+    LOGE("Failed to execute adb.\n");
+#else
+    switch (err) {
+    case 0:


may not happen

rom1v · 2018-09-03T19:24:23Z

app/src/command.c

@@ -18,9 +22,30 @@ static inline const char *get_adb_command() {
    return adb_command;
 }

+static void show_err_msg(int err) {
+#ifdef __WINDOWS__
+    LOGE("Failed to execute adb.\n");


Mostly windows users are lost with adb, isn't it possible to know when the problem is a missing adb?

rom1v · 2018-09-03T19:24:51Z

app/src/command.c

+        LOGE("Failed to execute adb.\n");
+        break;
+    case ENOENT:
+        LOGE("Missing adb. You need adb, accessible from your PATH to execute scrcpy.\n");


I suggest:

'adb' command not found (make it accessible from your PATH or define its absolute path in the ADB environment variable)

rom1v · 2018-09-03T19:25:11Z

app/src/command.c

+    case 0:
+        break;
+    case -1:
+        LOGE("Failed to execute adb.\n");


(for consistency, don't end with a ., and \n is not needed)

rom1v · 2018-09-03T19:27:27Z

app/src/sys/unix/command.c

+    int fd[2];
+    int ret = 0;
+
+    if (0 > pipe(fd)) {


if (pipe(fd) == -1) {

rom1v · 2018-09-03T19:28:15Z

app/src/sys/unix/command.c

+        close(fd[0]);
+        if (fcntl(fd[1], F_SETFD, FD_CLOEXEC) == -1) {
+            perror("fcntl");
+	    /* To prevent parent hang in read, we should exit when error. */


wrong indent (tab vs space)

rom1v · 2018-09-03T19:28:30Z

app/src/sys/unix/command.c

+            goto END;
+        }
+    } else if (*pid == 0) {
+        /* child close read side. */


(for consistency, use // for comments, and no ending .)

rom1v · 2018-09-03T19:37:41Z

app/src/command.c

+#ifdef __WINDOWS__
+    LOGE("Failed to execute adb.\n");
+#else
+    switch (err) {


(indent cases for consistency, it makes it more readable when cases have blocks)

rom1v · 2018-09-03T19:46:59Z

app/src/sys/unix/command.c

+        if (fcntl(fd[1], F_SETFD, FD_CLOEXEC) == -1) {
+            perror("fcntl");
+	    /* To prevent parent hang in read, we should exit when error. */
+            close(fd[1]);


must send a failure to the parent (e.g. errno set by fcntl), otherwise the parent will consider that the child has execed successfully

npes87184 · 2018-09-04T02:14:50Z

Hi, @rom1v

Thank you for your effort and time. All look good to me.

btw, I am not familiar with coding in Windows. I will take a look to survey how to detect it in Windows related system recently.

Sincerely,
yuchenlin

rom1v · 2018-09-04T06:49:22Z

Merged in dev: 6d2d803
I added another commit not to handle errno from the common command.c: ~~bf32f25~~ 55d33dd

I will take a look to survey how to detect it in Windows related system recently.

👍
If you have a solution, now you can just return PROCESS_ERROR_MISSING_BINARY 😉

check adb runnable before starting scrcpy

npes87184 force-pushed the dev branch from 5456e0a to b5cba56 Compare September 1, 2018 02:20

npes87184 force-pushed the dev branch 3 times, most recently from 861b5e7 to ebc5eba Compare September 1, 2018 02:47

rom1v reviewed Sep 2, 2018

View reviewed changes

Notify adb missing

12cd32c

There are many user who encounters missing adb. To stop things happens again, we check it and show sexy response to user. Signed-off-by: yuchenlin <[email protected]>

npes87184 force-pushed the dev branch from ebc5eba to 12cd32c Compare September 3, 2018 12:14

npes87184 changed the title ~~[RFC] check adb runnable before starting scrcpy~~ check adb runnable before starting scrcpy Sep 3, 2018

rom1v reviewed Sep 3, 2018

View reviewed changes

rom1v closed this Sep 4, 2018

rom1v added a commit that referenced this pull request Sep 4, 2018

Merge branch 'npes87184:dev' into dev (#236)

3455031

check adb runnable before starting scrcpy

npes87184 mentioned this pull request Sep 8, 2018

Return specific error for missing adb binary on Windows #254

Closed

npes87184 mentioned this pull request Oct 4, 2018

ERROR: Could not execute "adb push" #278

Closed

check adb runnable before starting scrcpy #236

check adb runnable before starting scrcpy #236

Conversation

npes87184 commented Aug 21, 2018

rom1v commented Aug 21, 2018

npes87184 commented Aug 21, 2018 • edited Loading

rom1v commented Aug 21, 2018

npes87184 commented Aug 21, 2018

npes87184 commented Sep 1, 2018

rom1v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npes87184 Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npes87184 commented Sep 3, 2018

v2 -> v3

rom1v left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npes87184 commented Sep 4, 2018

rom1v commented Sep 4, 2018 • edited Loading

npes87184 commented Aug 21, 2018 •

edited

Loading

npes87184 Sep 2, 2018 •

edited

Loading

rom1v left a comment •

edited

Loading

rom1v commented Sep 4, 2018 •

edited

Loading