termux / termux-tools

Scripts and small programs that are packaged into termux's termux-tools package
GNU General Public License v3.0
280 stars 60 forks source link

scripts/cmd: Replace shell script with implementation in c #111

Closed 2096779623 closed 1 month ago

2096779623 commented 1 month ago

CC @5ec1cff .

twaik commented 1 month ago

Is it supposed to do something like echo $(pm path com.termux 2>&1 </dev/null)? I mean using anonymous pipe to avoid selinux related problems?

2096779623 commented 1 month ago

Is it supposed to do something like echo $(pm path com.termux 2>&1 </dev/null)? I mean using anonymous pipe to avoid selinux related problems?

Yes.

agnostic-apollo commented 1 month ago
2096779623 commented 1 month ago
  • pump_stderr definition should be above set_fd() definition.
  • Separate blocks of actions done on each fd with a newline in both child and parent, like close/set_fd/dup2/pthread_create.
  • It's good practice to close fds explicitly after using them (in child after pumps).
  • The code will block on read() in pump_stdout() instead of waitpid() and after parent process exits, read() should return with a 0/EOF after all data parent wrote before exiting has been read, and then code will move to waitpid(). The issue here is that stderr may not have pumped completely before child exists after waitpid(). So after pump_stdout(), it should wait for stderr thread, like with pthread_join() (instead of using pthread_detach()). You can test by running some command that prints an error. This should not be necessary for stdin thread as if waitpid() returns, parent should have already read it completely if it needed to.

Done.

agnostic-apollo commented 1 month ago

Looks good, thanks. Maybe replace p2 with t_stdout and p_in with p_stdout, etc as currently p_in and p_out looks like some kind of input and output, instead of stdin/stdout.

Did you to test to see if stderr is fully printing? It should though.

agnostic-apollo commented 1 month ago

You didn't rename p_* variables, otherwise looks fine.

2096779623 commented 1 month ago

Did you to test to see if stderr is fully printing? It should though.

Yes,I did.

2096779623 commented 1 month ago

You didn't rename p_* variables, otherwise looks fine.

Done.

agnostic-apollo commented 1 month ago

Now you have added a _ after std, like p_std_out, so it's now different from t_stdout and pump_stdout. But you can merge anyways.