Re: [PATCH 3/6] Add new vDefaultInferiorFd packet

[Tom Tromey <tom@tromey.com>]

>>>>> "Alexandra" == Alexandra Hájková <ahajkova@redhat.com> writes:

Alexandra> +@item vDefaultInferior;@var{fd0};@var{fd1};@var{fd2}

I think the packet name is wrong on this line.

Alexandra> +@item vDefaultInferiorFd
Alexandra> +@anchor{default inferior file descriptors}
Alexandra> +@cindex @samp{vDefaultInferiorFd} packet
Alexandra> +@value{GDBN} would preserve the numbers of STDOUT/STDIN/STDERR file descriptors.
Alexandra> +Preserved numbers of the file descriptors are then sent to GDBserver.
Alexandra> +If GDBserver is run locally and will accept the numbers of the file
Alexandra> +descriptors, it will start the inferior connected to the same STDIN/OUT/ERR
Alexandra> +@value{GDBN} is connected to. This allows the inferior run under the local GDBserver

To me, the use of "sent" here makes it sound like this is using the
(obscure) SCM_RIGHTS feature, that lets one send file descriptors over a
socket.

However, it isn't, instead it is invoking gdbserver with some extra file
descriptors already opened, and then telling the server which ones to
use.

I think the text should be reworded to make this more clear.
 
Alexandra> +  /* Returns true if connecting inferior to the same terminal as GDB is
Alexandra> +     supported, false otherwise.  */
Alexandra> +  bool remote_fd_switch_p () const
Alexandra> +  { return packet_support (PACKET_vDefaultInferiorFd) == PACKET_ENABLE; }

This seems a little odd to me in that if I set up some kind of 'nc'
bridge and use it to connect to a remote valgrind-gdbserver, it will try
to use the gdb file descriptors but that just won't make sense at all.

I guess I'm wondering if this should try to use whatever mechanism we
came up with for checking if the remote is really remote.  I don't
remember if that ended up going in or not.

Alexandra> +  if ((m_features.packet_support (PACKET_vDefaultInferiorFd) != PACKET_DISABLE)
Alexandra> +      && (rs->remote_desc->fds[0] != -1) && (rs->remote_desc->fds[1] != -1)
Alexandra> +      && (rs->remote_desc->fds[2] != -1))

These are over-parenthesized.

Alexandra> +volatile int multi_mode = 0;

I think you can drop the volatile.  I suspect that was an artifact of
the old longjmp-based exception system.

Alexandra> @@ -4295,6 +4413,14 @@ process_serial_event (void)
Alexandra>    response_needed = true;
 
Alexandra>    char ch = cs.own_buf[0];
Alexandra> +  if (deferred_inferior_startup)
Alexandra> +  {
Alexandra> +    /* Actually start the inferior if not a qSupported or
Alexandra> +       vDefaultInferiorFd packet.  */
Alexandra> +    if (!startswith(cs.own_buf, "qSupported")
Alexandra> +       && !startswith(cs.own_buf, "vDefaultInferiorFd"))
Alexandra> +      do_deferred_startup ();
Alexandra> +  }

I had to read this a few times before I felt like it made sense.

Since there is an ordering dependency here, I think that should be
documented.  That is, vDefaultInferiorFd is only accepted after
qSupported and before any other request.

Tom