Re: [PATCH v2 1/2] Use a shell command as a socket for gdbserver

[Mike Frysinger <vapier at gentoo dot org>]

On 04 May 2015 23:54, Gabriel Corona wrote:
> +static int
> +open_shell_command (char *command)

const ?

> +{
> +  int sockets[2];
> +  pid_t child, grandchild, res;
> +  int status;
> +
> +  if (socketpair (AF_LOCAL, SOCK_STREAM, 0, sockets) < 0)
> +    perror_with_name ("Can't get socketpair");
> +  child = fork ();

prefer a blank line above the fork assignment

> +      if (dup2 (sockets[1], 0) < 0 || dup2 (sockets[1], 1) < 0)
> +	perror_with_name ("Can't dup socket to stdio");

use STDIN_FILENO & STDOUT_FILENO instead of 0 & 1

> +      if (close (sockets[1]) < 0)
> +	perror_with_name ("Can't close original socket");

dup2 has an edge case where if the fd is the same as an existing one, it won't 
create a new one.  i think before you close, you need to compare it to 
STDIN_FILENO & STDOUT_FILENO and only close it if it doesn't match.

> +      /* Double fork in order to inherit the grandchild. The process
> +	 is expected to exit when the other end of the socketpair is
> +	 closed.
> +       */

prefer a blank line above this comment to space the code out.  also, GNU style 
says:
 - use two spaces after periods
 - cuddle the trailing */ rather than putting it on a new line by itself

> +  else
> +    {
> +      close (sockets[1]);

check return value ?

> +      signal (SIGPIPE, SIG_IGN);
> +      while ((res = waitpid (child, &status, 0)) < 0 && errno == EINTR);
> +      if (res < 0)
> +	perror_with_name ("Can't wait child process");

phrasing is awkward ... maybe insert "on" or "for the" in there ?

> +     }
> +  return -1;
> +}

i think GNU style says you should have a blank line above this last return
-mike

Attachment: signature.asc
Description: Digital signature