Re: patch ping: QNX Neutrino remote debug support

[Andrew Cagney <ac131313 at redhat dot com>]

I obviously can't review the protocol implementation.  So ...

Two comming changes to keep in mind:

- having multiple live target stacks is on the horizon, that will meen 
that most statics will need to be consulated into a struct

- async is similar (assuming it can be re-started).  you'll want to 
think about the consequences.


anyway, some more general comments ...


> #include <fcntl.h>
> #include <signal.h>

"defs.h" is _always_ included first.  I also noticed that one of the 
other headers was pulling in <stdlib.h>?  Is it needed.

Is all of the following needed?  Look at how remote-io.c handled this 
problem.

> #ifdef __CYGWIN__
> #include <sys/cygwin.h>
> #endif
>
> #ifndef EOK
> #define EOK 0
> #endif
>
> #define QNX_READ_MODE	0x0
> #define QNX_WRITE_MODE	0x301
> #define QNX_WRITE_PERMS	0x1ff
>
> #ifdef __CYGWIN__
> #define HOST_READ_MODE  O_RDONLY|O_BINARY
> #define HOST_WRITE_MODE  O_WRONLY|O_CREAT|O_TRUNC|O_BINARY
> #else
> #define HOST_READ_MODE  O_RDONLY
> #define HOST_WRITE_MODE  O_WRONLY|O_CREAT|O_TRUNC
> #endif
>
> #ifndef O_BINARY
> #define O_BINARY 0
> #endif


> int (*target_created_hook) (pid_t);

GDB's trying to switch to observers, but first, is this even used?  Or 
is another hook/observer already available?


> #define SWAP64( val ) ( \
> #define SWAP32( val ) ( (((val) >> 24) & 0x000000ff)	\
> #define SWAP16( val ) ( (((val) >> 8) & 0xff) | (((val) << 8) & 0xff00) )
> nto_swap16 (int val)
> nto_swap32 (long int val)
> nto_swap64 (long long int val)

These all shouldn't be needed.  Instead extract_[un]signed_integer 
should be able to do the job (I'm guessing that this part of the 
protocol is target byteordered).

Use LONGEST if you need a large integer.

> #define SEND_NAK         serial_write(nto_desc,nak_packet,sizeof(nak_packet))
> #define SEND_CH_RESET    serial_write(nto_desc,ch_reset_packet,sizeof(ch_reset_packet))
> #define SEND_CH_DEBUG    serial_write(nto_desc,ch_debug_packet,sizeof(ch_debug_packet))
> #define SEND_CH_TEXT     serial_write(nto_desc,ch_text_packet,sizeof(ch_text_packet))

Shouldn't these be functions?

> 	    printf_unfiltered ("[escape]");

debug info should go to gdb_stdlog.

>   nto_desc = serial_open (name);

Ya!

[about here I maxed out]

> static void
> nto_add_commands ()
> {
>   struct cmd_list_element *c;
>
>   c =
>     add_com ("upload", class_obscure, upload_command,
> 	     "Send a file to the target (upload {local} [{remote}])");
>   c->completer = filename_completer;
>   add_com ("download", class_obscure, download_command,
> 	   "Get a file from the target (download {remote} [{local}])");
> }
>
> static void
> nto_remove_commands ()
> {
>   extern struct cmd_list_element *cmdlist;
>
>   delete_cmd ("upload", &cmdlist);
>   delete_cmd ("download", &cmdlist);
> }

Can you start a discussion about these commands (I guess on gdb@)?  (For 
the moment, leave them disabled?).

The revamping of "to_query" opens the potential for implementing this in 
a clean generic way.

> void
> _initialize_nto ()

Can this be moved to the end of the file?

>   add_show_from_set (add_set_cmd ("qnxtimeout", no_class,
> 				  var_integer, (char *) &nto_timeout,
> 				  "Set timeout value for remote read.\n",
> 				  &setlist), &showlist);
>
>   add_show_from_set (add_set_cmd ("qnxinheritenv", no_class,
> 				  var_boolean, (char *) &nto_inherit_env,
> 				  "Set where remote process inherits env from pdebug, or has it set by gdb\n",
> 				  &setlist), &showlist);
>
>   add_show_from_set (add_set_cmd ("qnxremotecwd", class_support, var_string,
> 				  (char *) &nto_remote_cwd,
> 				  "Set the working directory for the remote process",
> 				  &setlist), &showlist);
>
>   add_setshow_cmd ("nto-remote-file", class_files, var_string,
> 		   &nto_remote_file, "Set the remote file to be executed \
> when a user issues the 'run'\ncommand to a QNX \
> Neutrino remote target.\n", "Show the file to be executed on the remote\n\
> QNX Neutrino target.\n", NULL, NULL, &setlist, &showlist);
>
>   add_setshow_cmd ("upload-sets-exec", class_files, var_integer,
> 		   &upload_sets_exec,
> 		   "If set, upload will set nto_remote_file.\n",
> 		   "Show the flag for upload to set nto_remote_file.\n", NULL,
> 		   NULL, &setlist, &showlist);

See "remote.c" for how to set things up so that "set remote-nto ...", or 
"set nto ...?" works.  You'll, separatly, need doco.

For the "set qnx*", suggest, locally, investigating GDB's mechanism for 
deprecating commands.

BTW, "nto_remove_file" isn't meaningful.

>   add_info ("pidlist", nto_pidlist, "pidlist");
>   add_info ("meminfo", nto_meminfo, "memory information");

Can you explain these?  "info threads" vs "pidlist".  "maint info nto 
memory" or "info memory" vs "meminfo"?  As with "download", disable them 
for now.

Is the below needed?  I don't think it belongs in remote-nto as even a 
native will need to do this transformation.

> #ifndef __QNXNTO__
>
> #define QNXSIGHUP      1	/* Hangup.  */
> #define QNXSIGINT      2	/* Interrupt.  */
> #define QNXSIGQUIT     3	/* Quit.  */
> #define QNXSIGILL      4	/* Illegal instruction (not reset when caught).  */
> #define QNXSIGTRAP     5	/* Trace trap (not reset when caught).  */
> #define QNXSIGIOT      6	/* IOT instruction.  */
> #define QNXSIGABRT     6	/* Used by abort.  */

Andrew