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

check adb runnable before starting scrcpy #236

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions app/src/command.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
#include "command.h"

#ifdef __WINDOWS__
#else
Copy link
Collaborator

Choose a reason for hiding this comment

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

#ifndef __WINDOWS__

#include <errno.h>
#endif
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
Expand All @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

#else
switch (err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

case 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

may not happen

break;
case -1:
LOGE("Failed to execute adb.\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

I suggest:

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

break;
default:
LOGE("Failed to execute adb, errno: [%d].\n", err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(could print a user-friendly errno message with strerror())

break;
}
#endif
}

process_t adb_execute(const char *serial, const char *const adb_cmd[], int len) {
const char *cmd[len + 4];
int i;
int i, r;
process_t process;
cmd[0] = get_adb_command();
if (serial) {
cmd[1] = "-s";
Expand All @@ -32,7 +57,12 @@ 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;
return cmd_execute(cmd[0], cmd);
r = cmd_execute(cmd[0], cmd, &process);
if (r != 0) {
show_err_msg(r);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@npes87184 npes87184 Sep 2, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return PROCESS_NONE;

(otherwise Windows will not be happy 😉)

}
return process;
}

process_t adb_forward(const char *serial, uint16_t local_port, const char *device_socket_name) {
Expand Down
2 changes: 1 addition & 1 deletion app/src/command.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(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)

SDL_bool cmd_terminate(process_t pid);
SDL_bool cmd_simple_wait(process_t pid, exit_code_t *exit_code);

Expand Down
60 changes: 53 additions & 7 deletions app/src/sys/unix/command.c
Original file line number Diff line number Diff line change
@@ -1,24 +1,70 @@
#include "command.h"

#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include "log.h"

pid_t cmd_execute(const char *path, const char *const argv[]) {
pid_t pid = fork();
if (pid == -1) {
perror("fork");
int cmd_execute(const char *path, const char *const argv[], pid_t *pid) {
int fd[2];
int ret = 0;

if (0 > pipe(fd)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

perror("pipe");
return -1;
}
if (pid == 0) {

*pid = fork();
if (*pid == -1) {
perror("fork");
ret = -1;
goto END;
}

if (*pid > 0) {
/* parent close write side. */
close(fd[1]);
fd[1] = -1;
/* and blocking read until child exec or write some fail
* reason in fd. */
if (read(fd[0], &ret, sizeof(int)) == -1) {
perror("read");
ret = -1;
goto END;
}
} else if (*pid == 0) {
/* child close read side. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong indent (tab vs space)

close(fd[1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

_exit(1);
}
execvp(path, (char *const *)argv);
perror("exec");
/* if child execvp failed, before exit, we should write
* reason into the pipe. */
ret = errno;
if (write(fd[1], &ret, sizeof(int)) == -1) {
perror("write");
}
/* close write side before exiting. */
close(fd[1]);
_exit(1);
}
return pid;

END:
if (fd[0] != -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

may never happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes 👍

close(fd[0]);
}
if (fd[1] != -1) {
close(fd[1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be moved to the parent block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
return ret;
}

SDL_bool cmd_terminate(pid_t pid) {
Expand Down
11 changes: 7 additions & 4 deletions app/src/sys/win/command.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(same here)

STARTUPINFO si;
PROCESS_INFORMATION pi;
memset(&si, 0, sizeof(si));
Expand All @@ -18,7 +18,8 @@ HANDLE cmd_execute(const char *path, const char *const argv[]) {
size_t ret = xstrjoin(cmd, argv, ' ', sizeof(cmd));
if (ret >= sizeof(cmd)) {
LOGE("Command too long (%" PRIsizet " chars)", sizeof(cmd) - 1);
return NULL;
*handle = NULL;
return -1;
}

#ifdef WINDOWS_NOCONSOLE
Expand All @@ -27,10 +28,12 @@ HANDLE cmd_execute(const char *path, const char *const argv[]) {
int flags = 0;
#endif
if (!CreateProcess(NULL, cmd, NULL, NULL, FALSE, flags, NULL, NULL, &si, &pi)) {
return NULL;
*handle = NULL;
return -1;
}

return pi.hProcess;
*handle = pi.hProcess;
return 0;
}

SDL_bool cmd_terminate(HANDLE handle) {
Expand Down