Re: [PATCH 2/9 v2] Introduce nat/linux-namespaces.[ch]

[Pedro Alves <palves at redhat dot com>]

On 04/30/2015 01:05 PM, Gary Benson wrote:

...

> +   For avoidance of doubt, if the helper process receives a
> +   message it doesn't handle it will reply with MNSH_MSG_ERROR.
> +   If the main process receives MNSH_MSG_ERROR at any time then
> +   it will call internal_error.  If internal_error causes the
> +   main process to exit, the helper will notice this and also
> +   exit.  The helper will not exit until the main process
> +   terminates, so if the user continues through internal_error
> +   the helper will still be there awaiting requests from the
> +   main process.

...

> +/* Mount namespace helper message types.  */
> +
> +enum mnsh_msg_type
> +  {
> +    /* An unrecoverable communication error occurred.

I think "unrecoverable" here sounds a bit confusing, as it
contradicts the comment above that explains that the helper
is still awaiting requests if the user decides to continue
after internal_error.

> +       Receipt of this message by either end will cause
> +       an assertion failure in the main process.  */
> +    MNSH_MSG_ERROR,


> +    /* A request that the helper call unlink.  The single
> +       argument (the filename) should be passed in BUF, and
> +       should include a terminating NUL character.  Helper
> +       should respond with a MNSH_RET_INT.  */
> +    MNSH_REQ_UNLINK,
> +
> +    /* A request that the helper call readlink.  The single
> +       argument (the filename) should be passed in BUF, and
> +       should include a terminating NUL character. The helper

Missing double-space.

> +static void
> +mnsh_main (int sock)
> +{
> +  while (1)
> +    {
> +      enum mnsh_msg_type type;
> +      int fd, int1, int2;
> +      char buf[PATH_MAX];
> +      ssize_t size, response = -1;
> +
> +      size = mnsh_recv_message (sock, &type,
> +				&fd, &int1, &int2,
> +				buf, sizeof (buf));
> +
> +      if (size >= 0 && size < sizeof (buf))
> +	{
> +	  switch (type)
> +	    {
> +	    case MNSH_REQ_SETNS:
> +	      if (fd > 0)
> +		response = mnsh_handle_setns (sock, fd, int1);
> +	      break;
> +
> +	    case MNSH_REQ_OPEN:
> +	      if (buf[size - 1] == '\0')

Why these  == '\0' checks?  To protect against bugs?  In
that case, I guess this should be:

	      if (size > 0 && buf[size - 1] == '\0')

> +		response = mnsh_handle_open (sock, buf, int1, int2);
> +	      break;


And that's it.  Really all looks good to me.  :-)

Thanks,
Pedro Alves