Use external editor in 'commands' command

Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Hi all,

I have made a small patch to the 'commands' command to allow the use
of an external editor to add or modify commands. This is convenient if
you are dealing with many commands per breakpoint.
The editor follows the behavior of the 'edit' command (/bin/ex by
default, or the 'EDITOR' environment variable)

Tested on i686-pc-linux-gnu, please tell me what do you think about it.

2009-01-14 Alfredo Ortega <ortegaalfredo@gmail.com>
    * breakpoint.c (commands_command,_initialize_breakpoint):
    Add the 'edit' keyword to the 'commands' command to allow the
    use of an external editor to add or modify commands.

[-- Attachment #2: commands.diff --]
[-- Type: text/x-patch, Size: 3633 bytes --]

--- OLD/gdb/breakpoint.c	2009-01-14 17:46:26.000000000 -0200
+++ NEW/gdb/breakpoint.c	2009-01-14 19:32:40.000000000 -0200
@@ -585,14 +585,20 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+#define COMMANDS_EDCOMMAND "edit"
+
 static void
 commands_command (char *arg, int from_tty)
 {
   struct breakpoint *b;
   char *p;
-  int bnum;
+  int bnum,fsize;
   struct command_line *l;
-
+  char vitmp[50];
+  char cmdline[100];
+  FILE *tmpstream=NULL;
+  char *editor;
+  
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
@@ -602,6 +608,41 @@ commands_command (char *arg, int from_tt
 
   p = arg;
   bnum = get_number (&p);
+  vitmp[0]=0;
+  /* Edit commands with external editor */
+  if (!strcmp(COMMANDS_EDCOMMAND,p))	{
+	/* Generates the temporal file name*/
+	/* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
+	p=NULL;
+	strcpy(vitmp,"/tmp/.gdbXXXXXX");
+	if (mkstemp(vitmp)<0) return;
+	ALL_BREAKPOINTS (b)
+		if (b->number == bnum)
+			{
+			if (&b->commands)	{
+				/* commands exists, must dump them to the temporal file */
+				tmpstream=fopen(vitmp,"w");
+				l = b->commands;
+				while(l)	{
+					fsize=0;
+					fsize+=fwrite(l->line,1,strlen(l->line),tmpstream);
+					fsize+=fwrite("\n",1,strlen("\n"),tmpstream);
+					if (fsize<strlen(l->line)+1) {
+						fclose(tmpstream);
+						unlink(vitmp);
+						return;
+						};
+					l = l->next;
+					}
+				fclose(tmpstream);
+				}
+			/* Edit the file */
+			if ((editor = (char *) getenv ("EDITOR")) == NULL)
+				editor = "/bin/ex";
+			snprintf(cmdline,sizeof(cmdline),"%s \"%s\"",editor,vitmp);
+			if (system(cmdline)<0) return;
+    			}
+	}
 
   if (p && *p)
     error (_("Unexpected extra arguments following breakpoint number."));
@@ -609,17 +650,31 @@ commands_command (char *arg, int from_tt
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
       {
-	char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
+	if(vitmp[0])	{
+		/* redirect instream */
+		tmpstream=instream;
+		instream=fopen(vitmp,"r");
+		l = read_command_lines (NULL, from_tty, 1);
+		}
+	else	{
+		char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
 				 bnum);
-	struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
-	l = read_command_lines (tmpbuf, from_tty, 1);
-	do_cleanups (cleanups);
+		struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
+		l = read_command_lines (tmpbuf, from_tty, 1);
+		do_cleanups (cleanups);
+		}
 	free_command_lines (&b->commands);
 	b->commands = l;
 	breakpoints_changed ();
 	observer_notify_breakpoint_modified (b->number);
+	if(vitmp[0])	{
+		/* restore instream */
+		instream=tmpstream;
+		/* erase temporal file */
+		unlink(vitmp);
+		}
 	return;
-    }
+       }
   error (_("No breakpoint number %d."), bnum);
 }
 
@@ -8103,6 +8158,9 @@ Usage is `ignore N COUNT'."));
   add_com ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
+After the command number you can enter the `edit' keyword, and then you can \n\
+use the external editor to add or modify commands.\n\
+Uses EDITOR environment variable contents as editor (or ex as default).\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\

Re: Use external editor in 'commands' command

[Eli Zaretskii]

> Date: Wed, 14 Jan 2009 19:47:50 -0200
> From: Alfredo Ortega <ortegaalfredo@gmail.com>
> 
> I have made a small patch to the 'commands' command to allow the use
> of an external editor to add or modify commands. This is convenient if
> you are dealing with many commands per breakpoint.
> The editor follows the behavior of the 'edit' command (/bin/ex by
> default, or the 'EDITOR' environment variable)
> 
> Tested on i686-pc-linux-gnu, please tell me what do you think about it.

If this is accepted, we will need a suitable patch for the manual.

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

2009/1/14 Eli Zaretskii <eliz@gnu.org>
>
> > Date: Wed, 14 Jan 2009 19:47:50 -0200
> > From: Alfredo Ortega <ortegaalfredo@gmail.com>
> >
> > I have made a small patch to the 'commands' command to allow the use
> > of an external editor to add or modify commands. This is convenient if
> > you are dealing with many commands per breakpoint.
> > The editor follows the behavior of the 'edit' command (/bin/ex by
> > default, or the 'EDITOR' environment variable)
> >
> > Tested on i686-pc-linux-gnu, please tell me what do you think about it.
>
> If this is accepted, we will need a suitable patch for the manual.

I see. This updated patch contains my proposed documentation changes
to gdb.texinfo and refcard.tex. I hope that the patch format is
adequate

[-- Attachment #2: commands.diff --]
[-- Type: text/x-diff, Size: 5383 bytes --]

diff -upr OLD/gdb/breakpoint.c NEW/gdb/breakpoint.c
--- OLD/gdb/breakpoint.c	2009-01-14 17:46:26.000000000 -0200
+++ NEW/gdb/breakpoint.c	2009-01-14 19:32:40.000000000 -0200
@@ -585,14 +585,20 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+#define COMMANDS_EDCOMMAND "edit"
+
 static void
 commands_command (char *arg, int from_tty)
 {
   struct breakpoint *b;
   char *p;
-  int bnum;
+  int bnum,fsize;
   struct command_line *l;
-
+  char vitmp[50];
+  char cmdline[100];
+  FILE *tmpstream=NULL;
+  char *editor;
+  
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
@@ -602,6 +608,41 @@ commands_command (char *arg, int from_tt
 
   p = arg;
   bnum = get_number (&p);
+  vitmp[0]=0;
+  /* Edit commands with external editor */
+  if (!strcmp(COMMANDS_EDCOMMAND,p))	{
+	/* Generates the temporal file name*/
+	/* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
+	p=NULL;
+	strcpy(vitmp,"/tmp/.gdbXXXXXX");
+	if (mkstemp(vitmp)<0) return;
+	ALL_BREAKPOINTS (b)
+		if (b->number == bnum)
+			{
+			if (&b->commands)	{
+				/* commands exists, must dump them to the temporal file */
+				tmpstream=fopen(vitmp,"w");
+				l = b->commands;
+				while(l)	{
+					fsize=0;
+					fsize+=fwrite(l->line,1,strlen(l->line),tmpstream);
+					fsize+=fwrite("\n",1,strlen("\n"),tmpstream);
+					if (fsize<strlen(l->line)+1) {
+						fclose(tmpstream);
+						unlink(vitmp);
+						return;
+						};
+					l = l->next;
+					}
+				fclose(tmpstream);
+				}
+			/* Edit the file */
+			if ((editor = (char *) getenv ("EDITOR")) == NULL)
+				editor = "/bin/ex";
+			snprintf(cmdline,sizeof(cmdline),"%s \"%s\"",editor,vitmp);
+			if (system(cmdline)<0) return;
+    			}
+	}
 
   if (p && *p)
     error (_("Unexpected extra arguments following breakpoint number."));
@@ -609,17 +650,31 @@ commands_command (char *arg, int from_tt
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
       {
-	char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
+	if(vitmp[0])	{
+		/* redirect instream */
+		tmpstream=instream;
+		instream=fopen(vitmp,"r");
+		l = read_command_lines (NULL, from_tty, 1);
+		}
+	else	{
+		char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
 				 bnum);
-	struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
-	l = read_command_lines (tmpbuf, from_tty, 1);
-	do_cleanups (cleanups);
+		struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
+		l = read_command_lines (tmpbuf, from_tty, 1);
+		do_cleanups (cleanups);
+		}
 	free_command_lines (&b->commands);
 	b->commands = l;
 	breakpoints_changed ();
 	observer_notify_breakpoint_modified (b->number);
+	if(vitmp[0])	{
+		/* restore instream */
+		instream=tmpstream;
+		/* erase temporal file */
+		unlink(vitmp);
+		}
 	return;
-    }
+       }
   error (_("No breakpoint number %d."), bnum);
 }
 
@@ -8103,6 +8158,9 @@ Usage is `ignore N COUNT'."));
   add_com ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
+After the command number you can enter the `edit' keyword, and then you can \n\
+use the external editor to add or modify commands.\n\
+Uses EDITOR environment variable contents as editor (or ex as default).\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
diff -upr OLD/gdb/doc/gdb.texinfo NEW/gdb/doc/gdb.texinfo
--- OLD/gdb/doc/gdb.texinfo	2009-01-14 17:46:18.000000000 -0200
+++ NEW/gdb/doc/gdb.texinfo	2009-01-14 22:21:29.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands @r{[}@var{bnum}@r{]} edit
+This spawns an external editor for adding or editing commands. the final  @code{end} is not necessary in this case. @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5587,6 +5589,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
diff -upr OLD/gdb/doc/refcard.tex NEW/gdb/doc/refcard.tex
--- OLD/gdb/doc/refcard.tex	2009-01-14 17:46:18.000000000 -0200
+++ NEW/gdb/doc/refcard.tex	2009-01-14 22:21:35.000000000 -0200
@@ -355,10 +355,10 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands {\it n} \opt{{\it edit}} \par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default display}  \opt{{\tt edit} use external editor for commands}
+\cr
 end&end of {\it command-list}\cr
 \endsec
 

Re: Use external editor in 'commands' command

[Eli Zaretskii]

> Date: Wed, 14 Jan 2009 22:32:03 -0200
> From: Alfredo Ortega <ortegaalfredo@gmail.com>
> 
> I see. This updated patch contains my proposed documentation changes
> to gdb.texinfo and refcard.tex. I hope that the patch format is
> adequate.

Thanks you.

First, please also submit ChangeLog entries for your changes, both in
code and in documentation.

> +@item commands @r{[}@var{bnum}@r{]} edit
> +This spawns an external editor for adding or editing commands. the final  @code{end} is not necessary in this case. @xref{Choosing your Editor}.
                                                                  ^^^
Capital "T" here.

Also, please leave 2 blanks after a period that ends a sentence.

> +@node Choosing your Editor
>  @subsection Choosing your Editor

Adding a @node requires that you also modify the @menu in its parent
node, otherwise makeinfo will barf.

> +\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default display}  \opt{{\tt edit} use external editor for commands}
                                                                   ^^^
A period missing here.

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

2009/1/15 Eli Zaretskii <eliz@gnu.org>:
>> Date: Wed, 14 Jan 2009 22:32:03 -0200
>> From: Alfredo Ortega <ortegaalfredo@gmail.com>
>>
>> I see. This updated patch contains my proposed documentation changes
>> to gdb.texinfo and refcard.tex. I hope that the patch format is
>> adequate.
>
> Thanks you.
>
> First, please also submit ChangeLog entries for your changes, both in
> code and in documentation.
>
>> +@item commands @r{[}@var{bnum}@r{]} edit
>> +This spawns an external editor for adding or editing commands. the final  @code{end} is not necessary in this case. @xref{Choosing your Editor}.
>                                                                  ^^^
> Capital "T" here.
>
> Also, please leave 2 blanks after a period that ends a sentence.
>
>> +@node Choosing your Editor
>>  @subsection Choosing your Editor
>
> Adding a @node requires that you also modify the @menu in its parent
> node, otherwise makeinfo will barf.
>
>> +\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default display}  \opt{{\tt edit} use external editor for commands}
>                                                                   ^^^
> A period missing here.
>

Thanks for the corrections. Here are are both changelogs and the
updated diff, I hope there are less errors now...
I promise that my next patches will be better!

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
    * breakpoint.c (commands_command,_initialize_breakpoint):
    Add the 'edit' keyword to the 'commands' command to allow the
    use of an external editor to add or modify commands.

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
    * gdb.texinfo, refcard.tex (breakpoint commands): Added
documentation of the edit option, for editing commands with an
external editor.

[-- Attachment #2: commands.diff --]
[-- Type: text/x-patch, Size: 5844 bytes --]

diff -upr OLD/gdb/breakpoint.c NEW/gdb/breakpoint.c
--- OLD/gdb/breakpoint.c	2009-01-14 17:46:26.000000000 -0200
+++ NEW/gdb/breakpoint.c	2009-01-14 19:32:40.000000000 -0200
@@ -585,14 +585,20 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+#define COMMANDS_EDCOMMAND "edit"
+
 static void
 commands_command (char *arg, int from_tty)
 {
   struct breakpoint *b;
   char *p;
-  int bnum;
+  int bnum,fsize;
   struct command_line *l;
-
+  char vitmp[50];
+  char cmdline[100];
+  FILE *tmpstream=NULL;
+  char *editor;
+  
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
@@ -602,6 +608,41 @@ commands_command (char *arg, int from_tt
 
   p = arg;
   bnum = get_number (&p);
+  vitmp[0]=0;
+  /* Edit commands with external editor */
+  if (!strcmp(COMMANDS_EDCOMMAND,p))	{
+	/* Generates the temporal file name*/
+	/* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
+	p=NULL;
+	strcpy(vitmp,"/tmp/.gdbXXXXXX");
+	if (mkstemp(vitmp)<0) return;
+	ALL_BREAKPOINTS (b)
+		if (b->number == bnum)
+			{
+			if (&b->commands)	{
+				/* commands exists, must dump them to the temporal file */
+				tmpstream=fopen(vitmp,"w");
+				l = b->commands;
+				while(l)	{
+					fsize=0;
+					fsize+=fwrite(l->line,1,strlen(l->line),tmpstream);
+					fsize+=fwrite("\n",1,strlen("\n"),tmpstream);
+					if (fsize<strlen(l->line)+1) {
+						fclose(tmpstream);
+						unlink(vitmp);
+						return;
+						};
+					l = l->next;
+					}
+				fclose(tmpstream);
+				}
+			/* Edit the file */
+			if ((editor = (char *) getenv ("EDITOR")) == NULL)
+				editor = "/bin/ex";
+			snprintf(cmdline,sizeof(cmdline),"%s \"%s\"",editor,vitmp);
+			if (system(cmdline)<0) return;
+    			}
+	}
 
   if (p && *p)
     error (_("Unexpected extra arguments following breakpoint number."));
@@ -609,17 +650,31 @@ commands_command (char *arg, int from_tt
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
       {
-	char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
+	if(vitmp[0])	{
+		/* redirect instream */
+		tmpstream=instream;
+		instream=fopen(vitmp,"r");
+		l = read_command_lines (NULL, from_tty, 1);
+		}
+	else	{
+		char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
 				 bnum);
-	struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
-	l = read_command_lines (tmpbuf, from_tty, 1);
-	do_cleanups (cleanups);
+		struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
+		l = read_command_lines (tmpbuf, from_tty, 1);
+		do_cleanups (cleanups);
+		}
 	free_command_lines (&b->commands);
 	b->commands = l;
 	breakpoints_changed ();
 	observer_notify_breakpoint_modified (b->number);
+	if(vitmp[0])	{
+		/* restore instream */
+		instream=tmpstream;
+		/* erase temporal file */
+		unlink(vitmp);
+		}
 	return;
-    }
+       }
   error (_("No breakpoint number %d."), bnum);
 }
 
@@ -8103,6 +8158,9 @@ Usage is `ignore N COUNT'."));
   add_com ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
+After the command number you can enter the `edit' keyword, and then you can \n\
+use the external editor to add or modify commands.\n\
+Uses EDITOR environment variable contents as editor (or ex as default).\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
diff -upr OLD/gdb/doc/gdb.texinfo NEW/gdb/doc/gdb.texinfo
--- OLD/gdb/doc/gdb.texinfo	2009-01-14 17:46:18.000000000 -0200
+++ NEW/gdb/doc/gdb.texinfo	2009-01-16 03:54:37.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands @r{[}@var{bnum}@r{]} edit
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5384,6 +5386,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5587,6 +5590,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
diff -upr OLD/gdb/doc/refcard.tex NEW/gdb/doc/refcard.tex
--- OLD/gdb/doc/refcard.tex	2009-01-14 17:46:18.000000000 -0200
+++ NEW/gdb/doc/refcard.tex	2009-01-16 03:54:44.000000000 -0200
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands {\it n} \opt{{\it edit}} \par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec
 

Re: Use external editor in 'commands' command

[Eli Zaretskii]

> Date: Fri, 16 Jan 2009 05:38:41 -0200
> From: Alfredo Ortega <ortegaalfredo@gmail.com>
> 
> Thanks for the corrections. Here are are both changelogs and the
> updated diff, I hope there are less errors now...
> I promise that my next patches will be better!

No one is born with this, so there's nothing wrong in making such
minor mistakes.

I have several comments to your code:

> +  char vitmp[50];

vitmp[] is a file name, so 50 is not nearly enough characters to hold
the longest possible name.  At the very least, please use
FILENAME_MAX, or (better) some dynamic code that grows it as needed,
because some hosts (e.g., Hurd) don't have any limitations on file
name length.

> +  char cmdline[100];

Same here: 100 is not enough, because $EDITOR holds a file name.

> +  if (!strcmp(COMMANDS_EDCOMMAND,p))	{

This is not GNU style of laying out brace-delimited blocks; please use
the same style and indentation as elsewhere in GDB sources.

> +	strcpy(vitmp,"/tmp/.gdbXXXXXX");

Please leave a blank between the function name and the left
parenthesis, and also between the comma and the following argument in
argument lists.  Like this:

	 strcpy (vitmp, "/tmp/.gdbXXXXXX");

Btw, using "/tmp/.gdbXXXXXX" is non-portable to Windows, where there's
no guarantee there will be a "/tmp" on the current drive, and it will
not work at all in the DJGPP (a.k.a. DOS) port of GDB, because DOS
filesystems do not allow file names with a leading dot.

> +	/* Generates the temporal file name*/
                         ^^^^^^^^
"temporary"

> +	/* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */

What complaint do you see?

> +	if (mkstemp(vitmp)<0) return;

Are you using mkstemp from libiberty?  Because otherwise it may not be
available on the host.

> +					if (fsize<strlen(l->line)+1) {
> +						fclose(tmpstream);
> +						unlink(vitmp);
> +						return;
> +						};

Don't we want some error message in this case?

> +			if ((editor = (char *) getenv ("EDITOR")) == NULL)
> +				editor = "/bin/ex";

This is likewise non-portable: "/bin/ex" is only guaranteed to exist
on Posix platforms.

Re: Use external editor in 'commands' command

[Alfredo Ortega]

2009/1/16 Eli Zaretskii <eliz@gnu.org>:
>> Date: Fri, 16 Jan 2009 05:38:41 -0200
>> From: Alfredo Ortega <ortegaalfredo@gmail.com>
>>
>> Thanks for the corrections. Here are are both changelogs and the
>> updated diff, I hope there are less errors now...
>> I promise that my next patches will be better!
>
> No one is born with this, so there's nothing wrong in making such
> minor mistakes.
>
> I have several comments to your code:
>
>> +  char vitmp[50];
>
> vitmp[] is a file name, so 50 is not nearly enough characters to hold
> the longest possible name.  At the very least, please use
> FILENAME_MAX, or (better) some dynamic code that grows it as needed,
> because some hosts (e.g., Hurd) don't have any limitations on file
> name length.
>
>> +  char cmdline[100];
>
> Same here: 100 is not enough, because $EDITOR holds a file name.
>
>> +  if (!strcmp(COMMANDS_EDCOMMAND,p)) {
>
> This is not GNU style of laying out brace-delimited blocks; please use
> the same style and indentation as elsewhere in GDB sources.
>
>> +     strcpy(vitmp,"/tmp/.gdbXXXXXX");
>
> Please leave a blank between the function name and the left
> parenthesis, and also between the comma and the following argument in
> argument lists.  Like this:
>
>         strcpy (vitmp, "/tmp/.gdbXXXXXX");
>
> Btw, using "/tmp/.gdbXXXXXX" is non-portable to Windows, where there's
> no guarantee there will be a "/tmp" on the current drive, and it will
> not work at all in the DJGPP (a.k.a. DOS) port of GDB, because DOS
> filesystems do not allow file names with a leading dot.
>
>> +     /* Generates the temporal file name*/
>                         ^^^^^^^^
> "temporary"
>
>> +     /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
>
> What complaint do you see?
>
>> +     if (mkstemp(vitmp)<0) return;
>
> Are you using mkstemp from libiberty?  Because otherwise it may not be
> available on the host.
>
>> +                                     if (fsize<strlen(l->line)+1) {
>> +                                             fclose(tmpstream);
>> +                                             unlink(vitmp);
>> +                                             return;
>> +                                             };
>
> Don't we want some error message in this case?
>
>> +                     if ((editor = (char *) getenv ("EDITOR")) == NULL)
>> +                             editor = "/bin/ex";
>
> This is likewise non-portable: "/bin/ex" is only guaranteed to exist
> on Posix platforms.
>

In the next patch I will adress all those issues (The fixed lenght
buffers are ugly but you can never overflow them, the mkstemp template
and the snprintf don't allow for it, but then some bad truncation and
unpredictable execution may happen).

gcc will complain about tmpnamp() like this:

test.c:(.text+0x13): warning: the use of `tempnam' is dangerous,
better use `mkstemp'

The man page of FreeBSD explains it much better than the Debian one,
now I know it's because a possible race condition.
About the "/bin/ex" issue, to maintain consistency I did it the same
way that the EDIT command, here:

gdb/cli/cli-cmds.c:695

  if ((editor = (char *) getenv ("EDITOR")) == NULL)
      editor = "/bin/ex";

BTW "/bin/ex" exists on solaris 9, but not Debian or *BSD (is
/usr/bin/ex there), as a result text editing won't work by default on
most systems.
It would be difficult to choose a portable editor...maybe with some #defines

Re: Use external editor in 'commands' command

[Daniel Jacobowitz]

On Fri, Jan 16, 2009 at 08:08:06PM -0200, Alfredo Ortega wrote:
> About the "/bin/ex" issue, to maintain consistency I did it the same
> way that the EDIT command, here:
> 
> gdb/cli/cli-cmds.c:695
> 
>   if ((editor = (char *) getenv ("EDITOR")) == NULL)
>       editor = "/bin/ex";

If possible, please put this logic in just one place in the source.

It looked like your patch changed the behavior of the "commands"
command.  I don't think that's a good idea; it'll break things all
over the place.  Can this be a new command instead?  I'd suggest
"edit commands" except that already has a meaning; it'll try to
edit a source file containing a function named "commands".
Anyone got a better idea than edit-commands?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Use external editor in 'commands' command

[Tom Tromey]

>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:

Alfredo> It would be difficult to choose a portable editor...maybe
Alfredo> with some #defines

If you are interested in solving this problem, a user-settable
"editor" parameter would seem to be a reasonable thing to add.

There's also the option of simply not providing a default -- if EDITOR
is not set in the environment, we could make these commands call
error.

IMO you aren't required to do any better than already existing code in
gdb -- it would just be nice to have.  I do agree with Daniel that the
code in question should be pulled out into a utility function.

Tom

Re: Use external editor in 'commands' command

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> It looked like your patch changed the behavior of the "commands"
Daniel> command.  I don't think that's a good idea; it'll break things all
Daniel> over the place.

I think this patch changes it in a compatible way.
The user writes "commands 5 edit", which is currently an error.

It seems to me that it would also be compatible to make "commands" a
prefix command, and have "commands edit 5".  To me, this seems
somewhat more in keeping with other existing gdb commands.  What do
you think?


Alfredo, one other thing that I haven't seen mentioned is that a patch
this size will require copyright papers to be filed with the FSF.  If
you haven't done this yet, send me email off-list and I can get you
started.

Tom

Re: Use external editor in 'commands' command

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 16 Jan 2009 16:58:10 -0700
> 
> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
> 
> Daniel> It looked like your patch changed the behavior of the "commands"
> Daniel> command.  I don't think that's a good idea; it'll break things all
> Daniel> over the place.
> 
> I think this patch changes it in a compatible way.
> The user writes "commands 5 edit", which is currently an error.

But there's also just "commands" for the last breakpoint set.  Will
there be "commands edit" as well?

Re: Use external editor in 'commands' command

[Eli Zaretskii]

> Date: Fri, 16 Jan 2009 20:08:06 -0200
> From: Alfredo Ortega <ortegaalfredo@gmail.com>
> Cc: gdb-patches@sourceware.org
> 
> gcc will complain about tmpnamp() like this:
> 
> test.c:(.text+0x13): warning: the use of `tempnam' is dangerous,
> better use `mkstemp'

You say "tmpnam ()", but the GCC message is for `tempnam'.  Which one
is it?  These are different functions.

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

2009/1/17 Eli Zaretskii <eliz@gnu.org>:
>> Date: Fri, 16 Jan 2009 20:08:06 -0200
>> From: Alfredo Ortega <ortegaalfredo@gmail.com>
>> Cc: gdb-patches@sourceware.org
>>
>> gcc will complain about tmpnamp() like this:
>>
>> test.c:(.text+0x13): warning: the use of `tempnam' is dangerous,
>> better use `mkstemp'
>
> You say "tmpnam ()", but the GCC message is for `tempnam'.  Which one
> is it?  These are different functions.
>

You are right, I screwed with the test, but the error is the same:

/home/alfred/gdb/gdb-6.8/src/gdb/breakpoint.c:609: warning: the use of
`tmpnam' is dangerous, better use `mkstemp'

I'm posting my latest patch, there are multiple changes:

1) Following the suggestion of Tom Tromey, i made "commands" a prefix
command, and now it is "commands edit n". The change is still
backwards compatible. (Didn't made it to auto-complete like other
commands...need to read a little on how to do this)
2) Now there is a "set external-editor" and "show external-editor",
this variable has precedence over the "EDITOR" environment variable,
(If not set, GDB follows the old behavior with "EDITOR" and "/bin/ex")
3) There is a external_editor() utility function in utils.c that one
should call if needing an external text editor.
4) Following the suggestions of Eli, now I use an auxiliary function
of libiberty for temporary file generation, all strings are dynamic
and there is much better error reporting.

This is a much better patch, but also is a much bigger one (I already
sent the FSF form that Tom suggested), so surely there are plenty of
errors. Corrections are welcomed.
Regards,

Alfredo



2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
   * breakpoint.c (commands_command,_initialize_breakpoint):
   Add the 'edit' keyword to the 'commands' command to allow the
   use of an external editor to add or modify commands.
   * utils.c,defs.h (external_editor,initialize_utils):
   Added an utility function to return the external text editor of the system.
   Added "set external-editor" and "show external-editor" commands to
set/show the external editor variable

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
   * gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option, for editing commands with an
external editor. Also, added a brief description of the "set external-editor"
and "show external-editor" commands.

[-- Attachment #2: commands.diff --]
[-- Type: text/x-diff, Size: 9259 bytes --]

diff -upr OLD/src/gdb/breakpoint.c NEW/src/gdb/breakpoint.c
--- OLD/src/gdb/breakpoint.c	2009-01-14 03:10:29.000000000 -0200
+++ NEW/src/gdb/breakpoint.c	2009-01-19 09:36:06.000000000 -0200
@@ -585,13 +585,20 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+#define COMMANDS_EDCOMMAND "edit"
+
 static void
 commands_command (char *arg, int from_tty)
 {
   struct breakpoint *b;
   char *p;
-  int bnum;
+  int bnum, fsize;
   struct command_line *l;
+  char *vitmp = NULL;
+  char *cmdline = NULL;
+  FILE *tmpstream = NULL;
+  char *editor;
+  int sysret;
 
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
@@ -599,30 +606,102 @@ commands_command (char *arg, int from_tt
 
   if (executing_breakpoint_commands)
     error (_("Can't use the \"commands\" command among a breakpoint's commands."));
-
   p = arg;
-  bnum = get_number (&p);
-
+  /* Edit commands with external editor */
+  if (p && (!strncmp (COMMANDS_EDCOMMAND, p, strlen (COMMANDS_EDCOMMAND))))
+    {
+      /* discard the "edit" command */
+      get_number (&p);
+      bnum = get_number (&p);
+      /* Generates the temporary file name */
+      /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
+      p = NULL;
+      if (!(vitmp = make_temp_file (NULL)))
+	{
+	  error (_("Can't create temporary file for editing."));
+	  return;
+	}
+      if ((editor = external_editor ()) == NULL)
+	{
+	  error (_("External editor not found."));
+	  return;
+	}
+      ALL_BREAKPOINTS (b) if (b->number == bnum)
+	{
+	  if (&b->commands)
+	    {
+	      /* commands exists, must dump them to the temporal file */
+	      tmpstream = fopen (vitmp, "w");
+	      l = b->commands;
+	      while (l)
+		{
+		  fsize = 0;
+		  fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);
+		  fsize += fwrite ("\n", 1, strlen ("\n"), tmpstream);
+		  if (fsize < strlen (l->line) + 1)
+		    {
+		      error (_("Error writing to temporary file."));
+		      fclose (tmpstream);
+		      unlink (vitmp);
+		      return;
+		    };
+		  l = l->next;
+		}
+	      fclose (tmpstream);
+	    }
+	  /* Edit the file */
+	  cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
+	  sprintf (cmdline, "%s \"%s\"", editor, vitmp);
+	  sysret = system (cmdline);
+	  xfree (cmdline);
+	  if (sysret < 0)
+	    {
+	      error (_("Editor command failed."));
+	      return;
+	    }
+	}
+    }
+  else
+    bnum = get_number (&p);
   if (p && *p)
     error (_("Unexpected extra arguments following breakpoint number."));
 
-  ALL_BREAKPOINTS (b)
-    if (b->number == bnum)
-      {
-	char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit, one per line.", 
-				 bnum);
-	struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
-	l = read_command_lines (tmpbuf, from_tty, 1);
-	do_cleanups (cleanups);
-	free_command_lines (&b->commands);
-	b->commands = l;
-	breakpoints_changed ();
-	observer_notify_breakpoint_modified (b->number);
-	return;
+  ALL_BREAKPOINTS (b) if (b->number == bnum)
+    {
+      if (vitmp)
+	{
+	  /* redirect instream */
+	  tmpstream = instream;
+	  instream = fopen (vitmp, "r");
+	  l = read_command_lines (NULL, from_tty, 1);
+	}
+      else
+	{
+	  char *tmpbuf =
+	    xstrprintf
+	    ("Type commands for when breakpoint %d is hit, one per line.",
+	     bnum);
+	  struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
+	  l = read_command_lines (tmpbuf, from_tty, 1);
+	  do_cleanups (cleanups);
+	}
+      free_command_lines (&b->commands);
+      b->commands = l;
+      breakpoints_changed ();
+      observer_notify_breakpoint_modified (b->number);
+      if (vitmp)
+	{
+	  /* restore instream */
+	  instream = tmpstream;
+	  /* erase temporal file */
+	  unlink (vitmp);
+	}
+      return;
     }
   error (_("No breakpoint number %d."), bnum);
 }
 
+
 /* Like commands_command, but instead of reading the commands from
    input stream, takes them from an already parsed command structure.
 
@@ -8103,6 +8182,10 @@ Usage is `ignore N COUNT'."));
   add_com ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
+Before the command number you can enter the `edit' keyword, and then you can \n\
+use the external editor to add or modify commands.\n\
+Uses the external-editor variable, EDITOR environment variable or /bin/ex,\n\
+in that precedence.\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
diff -upr OLD/src/gdb/defs.h NEW/src/gdb/defs.h
--- OLD/src/gdb/defs.h	2009-01-14 03:10:28.000000000 -0200
+++ NEW/src/gdb/defs.h	2009-01-19 08:57:04.000000000 -0200
@@ -330,6 +330,8 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
+extern char *external_editor( void );
+
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
diff -upr OLD/src/gdb/doc/gdb.texinfo NEW/src/gdb/doc/gdb.texinfo
--- OLD/src/gdb/doc/gdb.texinfo	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/gdb.texinfo	2009-01-19 09:26:37.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5384,6 +5386,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5587,6 +5590,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
@@ -5611,6 +5615,19 @@ or in the @code{csh} shell,
 setenv EDITOR /usr/bin/vi
 gdb @dots{}
 @end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use. 
+@end table
 
 @node Search
 @section Searching Source Files
diff -upr OLD/src/gdb/doc/refcard.tex NEW/src/gdb/doc/refcard.tex
--- OLD/src/gdb/doc/refcard.tex	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/refcard.tex	2009-01-19 08:57:19.000000000 -0200
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec
 
diff -upr OLD/src/gdb/utils.c NEW/src/gdb/utils.c
--- OLD/src/gdb/utils.c	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/utils.c	2009-01-19 09:25:19.000000000 -0200
@@ -96,6 +96,10 @@ static void prompt_for_continue (void);
 static void set_screen_size (void);
 static void set_width (void);
 
+/* External text editor */
+
+static char *external_editor_command = NULL;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 
 static int debug_timestamp = 0;
@@ -2694,6 +2698,12 @@ When set, debugging messages will be mar
 			   NULL,
 			   show_debug_timestamp,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+			    _("\
+Show the external text editor."), NULL,
+			    NULL, NULL,
+			    &setlist, &showlist);
 }
 
 /* Machine specific function to handle SIGWINCH signal. */
@@ -3443,3 +3453,16 @@ gdb_buildargv (const char *s)
     nomem (0);
   return argv;
 }
+
+/* Returns the external editor */
+char *
+external_editor (void)
+{
+  char *editor;
+  if (external_editor_command)
+    return external_editor_command;
+  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+    editor = "/bin/ex";
+  return editor;
+
+}

Re: Use external editor in 'commands' command

[dgutson]

Hi Alfredo,

  I find the function too long. You might want to split it in more
functions, such as one to export a bp to a text file, another one to import
it, etc., so this function becomes more modular.

My 2 cents.

  Daniel.


Alfredo Ortega wrote:
> 
> 2009/1/14 Eli Zaretskii <eliz@gnu.org>
>>
>> > Date: Wed, 14 Jan 2009 19:47:50 -0200
>> > From: Alfredo Ortega <ortegaalfredo@gmail.com>
>> >
>> > I have made a small patch to the 'commands' command to allow the use
>> > of an external editor to add or modify commands. This is convenient if
>> > you are dealing with many commands per breakpoint.
>> > The editor follows the behavior of the 'edit' command (/bin/ex by
>> > default, or the 'EDITOR' environment variable)
>> >
>> > Tested on i686-pc-linux-gnu, please tell me what do you think about it.
>>
>> If this is accepted, we will need a suitable patch for the manual.
> 
> I see. This updated patch contains my proposed documentation changes
> to gdb.texinfo and refcard.tex. I hope that the patch format is
> adequate
> 
> diff -upr OLD/gdb/breakpoint.c NEW/gdb/breakpoint.c
> --- OLD/gdb/breakpoint.c	2009-01-14 17:46:26.000000000 -0200
> +++ NEW/gdb/breakpoint.c	2009-01-14 19:32:40.000000000 -0200
> @@ -585,14 +585,20 @@ condition_command (char *arg, int from_t
>    error (_("No breakpoint number %d."), bnum);
>  }
>  
> +#define COMMANDS_EDCOMMAND "edit"
> +
>  static void
>  commands_command (char *arg, int from_tty)
>  {
>    struct breakpoint *b;
>    char *p;
> -  int bnum;
> +  int bnum,fsize;
>    struct command_line *l;
> -
> +  char vitmp[50];
> +  char cmdline[100];
> +  FILE *tmpstream=NULL;
> +  char *editor;
> +  
>    /* If we allowed this, we would have problems with when to
>       free the storage, if we change the commands currently
>       being read from.  */
> @@ -602,6 +608,41 @@ commands_command (char *arg, int from_tt
>  
>    p = arg;
>    bnum = get_number (&p);
> +  vitmp[0]=0;
> +  /* Edit commands with external editor */
> +  if (!strcmp(COMMANDS_EDCOMMAND,p))	{
> +	/* Generates the temporal file name*/
> +	/* vitmp = tempnam(NULL,".gdb"); this is more secure according to man
> mkstemp, but gcc complains... */
> +	p=NULL;
> +	strcpy(vitmp,"/tmp/.gdbXXXXXX");
> +	if (mkstemp(vitmp)<0) return;
> +	ALL_BREAKPOINTS (b)
> +		if (b->number == bnum)
> +			{
> +			if (&b->commands)	{
> +				/* commands exists, must dump them to the temporal file */
> +				tmpstream=fopen(vitmp,"w");
> +				l = b->commands;
> +				while(l)	{
> +					fsize=0;
> +					fsize+=fwrite(l->line,1,strlen(l->line),tmpstream);
> +					fsize+=fwrite("\n",1,strlen("\n"),tmpstream);
> +					if (fsize<strlen(l->line)+1) {
> +						fclose(tmpstream);
> +						unlink(vitmp);
> +						return;
> +						};
> +					l = l->next;
> +					}
> +				fclose(tmpstream);
> +				}
> +			/* Edit the file */
> +			if ((editor = (char *) getenv ("EDITOR")) == NULL)
> +				editor = "/bin/ex";
> +			snprintf(cmdline,sizeof(cmdline),"%s \"%s\"",editor,vitmp);
> +			if (system(cmdline)<0) return;
> +    			}
> +	}
>  
>    if (p && *p)
>      error (_("Unexpected extra arguments following breakpoint number."));
> @@ -609,17 +650,31 @@ commands_command (char *arg, int from_tt
>    ALL_BREAKPOINTS (b)
>      if (b->number == bnum)
>        {
> -	char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is hit,
> one per line.", 
> +	if(vitmp[0])	{
> +		/* redirect instream */
> +		tmpstream=instream;
> +		instream=fopen(vitmp,"r");
> +		l = read_command_lines (NULL, from_tty, 1);
> +		}
> +	else	{
> +		char *tmpbuf = xstrprintf ("Type commands for when breakpoint %d is
> hit, one per line.", 
>  				 bnum);
> -	struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
> -	l = read_command_lines (tmpbuf, from_tty, 1);
> -	do_cleanups (cleanups);
> +		struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
> +		l = read_command_lines (tmpbuf, from_tty, 1);
> +		do_cleanups (cleanups);
> +		}
>  	free_command_lines (&b->commands);
>  	b->commands = l;
>  	breakpoints_changed ();
>  	observer_notify_breakpoint_modified (b->number);
> +	if(vitmp[0])	{
> +		/* restore instream */
> +		instream=tmpstream;
> +		/* erase temporal file */
> +		unlink(vitmp);
> +		}
>  	return;
> -    }
> +       }
>    error (_("No breakpoint number %d."), bnum);
>  }
>  
> @@ -8103,6 +8158,9 @@ Usage is `ignore N COUNT'."));
>    add_com ("commands", class_breakpoint, commands_command, _("\
>  Set commands to be executed when a breakpoint is hit.\n\
>  Give breakpoint number as argument after \"commands\".\n\
> +After the command number you can enter the `edit' keyword, and then you
> can \n\
> +use the external editor to add or modify commands.\n\
> +Uses EDITOR environment variable contents as editor (or ex as
> default).\n\
>  With no argument, the targeted breakpoint is the last one set.\n\
>  The commands themselves follow starting on the next line.\n\
>  Type a line containing \"end\" to indicate the end of them.\n\
> diff -upr OLD/gdb/doc/gdb.texinfo NEW/gdb/doc/gdb.texinfo
> --- OLD/gdb/doc/gdb.texinfo	2009-01-14 17:46:18.000000000 -0200
> +++ NEW/gdb/doc/gdb.texinfo	2009-01-14 22:21:29.000000000 -0200
> @@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
>  With no @var{bnum} argument, @code{commands} refers to the last
>  breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
>  recently encountered).
> +@item commands @r{[}@var{bnum}@r{]} edit
> +This spawns an external editor for adding or editing commands. the final 
> @code{end} is not necessary in this case. @xref{Choosing your Editor}.
>  @end table
>  
>  Pressing @key{RET} as a means of repeating the last @value{GDBN} command
> is
> @@ -5587,6 +5589,7 @@ Edit the file containing @var{function} 
>  
>  @end table
>  
> +@node Choosing your Editor
>  @subsection Choosing your Editor
>  You can customize @value{GDBN} to use any editor you want
>  @footnote{
> diff -upr OLD/gdb/doc/refcard.tex NEW/gdb/doc/refcard.tex
> --- OLD/gdb/doc/refcard.tex	2009-01-14 17:46:18.000000000 -0200
> +++ NEW/gdb/doc/refcard.tex	2009-01-14 22:21:35.000000000 -0200
> @@ -355,10 +355,10 @@ delete when reached
>  ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
>  times\cr
>  \cr
> -commands {\it n}\par
> +commands {\it n} \opt{{\it edit}} \par
>  \qquad \opt{\tt silent}\par
> -\qquad {\it command-list}&execute GDB {\it command-list} every time
> breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
> -display}\cr
> +\qquad {\it command-list}&execute GDB {\it command-list} every time
> breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
> display}  \opt{{\tt edit} use external editor for commands}
> +\cr
>  end&end of {\it command-list}\cr
>  \endsec
>  
> 
> 

-- 
View this message in context: http://www.nabble.com/Use-external-editor-in-%27commands%27-command-tp21465971p21593783.html
Sent from the Sourceware - gdb-patches mailing list archive at Nabble.com.

Re: Use external editor in 'commands' command

[Tom Tromey]

>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:

Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
Alfredo> a prefix command, and now it is "commands edit n".

Your patch does this using strncmp -- but gdb already has built-in
machinery for prefix commands.  See add_prefix_cmd.  So, the idea here
would be to change _initialize_breakpoint to use add_prefix_cmd,
instead of add_com, when creating the "commands" command.  Then, you'd
have a separate function to implement "commands edit".  Maybe this
means introducing a helper function to do some of the work, I don't
know.

Alfredo> This is a much better patch, but also is a much bigger one (I already
Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
Alfredo> errors. Corrections are welcomed.

A few nits inline.

Alfredo> +#define COMMANDS_EDCOMMAND "edit"

With the prefix change, you won't need this.

Alfredo> +  /* Edit commands with external editor */

In the GNU style, comments are full sentences that start with a
capital letter and end with a period and two spaces.  This one is
missing the period, but others are incorrect in other ways.

Alfredo> +      /* discard the "edit" command */

E.g., this one...

Alfredo> +      get_number (&p);
Alfredo> +      bnum = get_number (&p);

Two calls to get_number seems suspect.

Alfredo> +      /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */

There's no need for this comment, IMO.

Alfredo> +      if (!(vitmp = make_temp_file (NULL)))

GNU style prohibits assignments in conditionals.

Alfredo> +	{
Alfredo> +	  error (_("Can't create temporary file for editing."));
Alfredo> +	  return;
Alfredo> +	}

The "error" function never returns.  It calls longjmp.  So, this
return is not needed.  This occurs in a few spots.

Alfredo> +	      l = b->commands;
Alfredo> +	      while (l)
Alfredo> +		{
Alfredo> +		  fsize = 0;
Alfredo> +		  fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);

I think you should probably use "print_command_lines" to print the
breakpoint commands to a file.

Alfredo> +	  sysret = system (cmdline);
Alfredo> +	  xfree (cmdline);
Alfredo> +	  if (sysret < 0)

I think this should also check "sysret" when it is >= 0, and fail if
the editor does not exit with status 0.

Thanks for working on this,
Tom

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]

2009/2/2 Tom Tromey <tromey@redhat.com>:
>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:
>
> Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
> Alfredo> a prefix command, and now it is "commands edit n".
>
> Your patch does this using strncmp -- but gdb already has built-in
> machinery for prefix commands.  See add_prefix_cmd.  So, the idea here
> would be to change _initialize_breakpoint to use add_prefix_cmd,
> instead of add_com, when creating the "commands" command.  Then, you'd
> have a separate function to implement "commands edit".  Maybe this
> means introducing a helper function to do some of the work, I don't
> know.
>
> Alfredo> This is a much better patch, but also is a much bigger one (I already
> Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
> Alfredo> errors. Corrections are welcomed.
>
> A few nits inline.
>
> Alfredo> +#define COMMANDS_EDCOMMAND "edit"
>
> With the prefix change, you won't need this.
>
> Alfredo> +  /* Edit commands with external editor */
>
> In the GNU style, comments are full sentences that start with a
> capital letter and end with a period and two spaces.  This one is
> missing the period, but others are incorrect in other ways.
>
> Alfredo> +      /* discard the "edit" command */
>
> E.g., this one...
>
> Alfredo> +      get_number (&p);
> Alfredo> +      bnum = get_number (&p);
>
> Two calls to get_number seems suspect.
>
> Alfredo> +      /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
>
> There's no need for this comment, IMO.
>
> Alfredo> +      if (!(vitmp = make_temp_file (NULL)))
>
> GNU style prohibits assignments in conditionals.
>
> Alfredo> +      {
> Alfredo> +        error (_("Can't create temporary file for editing."));
> Alfredo> +        return;
> Alfredo> +      }
>
> The "error" function never returns.  It calls longjmp.  So, this
> return is not needed.  This occurs in a few spots.
>
> Alfredo> +            l = b->commands;
> Alfredo> +            while (l)
> Alfredo> +              {
> Alfredo> +                fsize = 0;
> Alfredo> +                fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);
>
> I think you should probably use "print_command_lines" to print the
> breakpoint commands to a file.
>
> Alfredo> +        sysret = system (cmdline);
> Alfredo> +        xfree (cmdline);
> Alfredo> +        if (sysret < 0)
>
> I think this should also check "sysret" when it is >= 0, and fail if
> the editor does not exit with status 0.
>
> Thanks for working on this,
> Tom
>

Sorry for the late answer.
I refactored the whole patch, is getting better...add_prefix_cmd()
made me sweat! :) and print_command_lines() is a little tricky to use,
so maybe i didn't get it right this time.
Also I send the FSF paperwork and tried to do all your corrections in
the following patch.
Thanks to you for teaching and the patience!

Regards,

Alfred

[-- Attachment #2: commands2.diff --]
[-- Type: text/x-diff, Size: 9932 bytes --]

diff -upr OLD/src/gdb/breakpoint.c NEW/src/gdb/breakpoint.c
--- OLD/src/gdb/breakpoint.c	2009-01-14 03:10:29.000000000 -0200
+++ NEW/src/gdb/breakpoint.c	2009-02-19 06:12:26.000000000 -0200
@@ -45,6 +45,7 @@
 #include "completer.h"
 #include "gdb.h"
 #include "ui-out.h"
+#include "cli-out.h"
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
 #include "block.h"
@@ -585,20 +586,116 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+/* Dumps breakpoint commands to a file.  */
 static void
-commands_command (char *arg, int from_tty)
+commands_dump_to_file (char *filename, struct breakpoint *b)
+{
+  struct cleanup *cleanups;
+  struct ui_file *old, *outfile = gdb_fopen (filename, "w");
+  cleanups = make_cleanup_ui_file_delete (outfile);
+  old = cli_out_set_stream (uiout, outfile);
+  print_command_lines (uiout, b->commands, 4);
+  cli_out_set_stream (uiout, old);
+  do_cleanups (cleanups);
+
+}
+
+/* Launches the editor on the breakpoint command.  */
+char *
+commands_edit_command (int bnum)
 {
+  char *vitmp = NULL;
+  char *editor;
   struct breakpoint *b;
-  char *p;
-  int bnum;
-  struct command_line *l;
+  char *cmdline = NULL;
+  int sysret;
+  /* Generates the temporary file name.  */
+  vitmp = make_temp_file (NULL);
+  if (!vitmp)
+      error (_("Can't create temporary file for editing."));
+  editor = external_editor ();
+  if (!editor)
+      error (_("External editor not found."));
+  ALL_BREAKPOINTS (b) if (b->number == bnum)
+    {
+      if (&b->commands)
+	commands_dump_to_file (vitmp, b);
+      /* Edit the file.  */
+      cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
+      sprintf (cmdline, "%s \"%s\"", editor, vitmp);
+      sysret = system (cmdline);
+      xfree (cmdline);
+      if (sysret < 0)
+	  error (_("Can't execute external editor."));
+      if (sysret > 0)
+	  error (_("External editor returned non-zero status."));
+    }
+  return vitmp;
+}
+
 
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
 
+static void
+check_executing_commands ()
+{
   if (executing_breakpoint_commands)
-    error (_("Can't use the \"commands\" command among a breakpoint's commands."));
+    error (_
+	   ("Can't use the \"commands\" command among a breakpoint's commands."));
+}
+
+/* Like commands_command but using an external editor.  */
+static void
+edit_command (char *args, int from_tty)
+{
+  int bnum;
+  char *p;
+  char *vitmp;
+  struct breakpoint *b;
+  struct command_line *l;
+  FILE *tmpstream = NULL;
+
+  check_executing_commands ();
+  p = args;
+  if (!p)
+    error (_("No breakpoint number."));
+  bnum = get_number (&p);
+  if (p && *p)
+    error (_("Unexpected extra arguments following breakpoint number."));
+  /* Launches the editor.  */
+  vitmp = commands_edit_command (bnum);
+  if (!vitmp)
+    return;
+
+  ALL_BREAKPOINTS (b) if (b->number == bnum)
+    {
+      /* Redirect instream to the commands temporal file.  */
+      tmpstream = instream;
+      instream = fopen (vitmp, "r");
+      l = read_command_lines (NULL, from_tty, 1);
+      free_command_lines (&b->commands);
+      b->commands = l;
+      breakpoints_changed ();
+      observer_notify_breakpoint_modified (b->number);
+      /* Restore instream and erase temporal file.  */
+      instream = tmpstream;
+      unlink (vitmp);
+      return;
+    }
+  error (_("No breakpoint number %d."), bnum);
+}
+
+static void
+commands_command (char *arg, int from_tty)
+{
+  struct breakpoint *b;
+  char *p;
+  int bnum;
+  struct command_line *l;
+
+  check_executing_commands();
 
   p = arg;
   bnum = get_number (&p);
@@ -8052,6 +8149,12 @@ Multiple breakpoints at one place are pe
 \n\
 Do \"help breakpoints\" for info on other commands dealing with breakpoints."
 
+/* List of subcommands for "commands".  */
+static struct cmd_list_element *commands_command_list;
+
+/* List of subcommands for "edit".  */
+static struct cmd_list_element *edit_command_list;
+
 /* List of subcommands for "catch".  */
 static struct cmd_list_element *catch_cmdlist;
 
@@ -8100,14 +8203,20 @@ Usage is `ignore N COUNT'."));
   if (xdb_commands)
     add_com_alias ("bc", "ignore", class_breakpoint, 1);
 
-  add_com ("commands", class_breakpoint, commands_command, _("\
+  add_prefix_cmd ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
 Give \"silent\" as the first line to make the breakpoint silent;\n\
-then no output is printed when it is hit, except what the commands print."));
+then no output is printed when it is hit, except what the commands print."),
+		      &commands_command_list, "commands ", 1, &cmdlist);
+
+  add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\
+Edit the command using the external-editor variable, EDITOR environment\n\
+variable or /bin/ex, in that precedence."),
+		      &edit_command_list, "command edit ", 1, &commands_command_list);
 
   add_com ("condition", class_breakpoint, condition_command, _("\
 Specify breakpoint number N to break only if COND is true.\n\
diff -upr OLD/src/gdb/defs.h NEW/src/gdb/defs.h
--- OLD/src/gdb/defs.h	2009-01-14 03:10:28.000000000 -0200
+++ NEW/src/gdb/defs.h	2009-01-19 08:57:04.000000000 -0200
@@ -330,6 +330,8 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
+extern char *external_editor( void );
+
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
diff -upr OLD/src/gdb/doc/gdb.texinfo NEW/src/gdb/doc/gdb.texinfo
--- OLD/src/gdb/doc/gdb.texinfo	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/gdb.texinfo	2009-01-19 09:26:37.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5384,6 +5386,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5587,6 +5590,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
@@ -5611,6 +5615,19 @@ or in the @code{csh} shell,
 setenv EDITOR /usr/bin/vi
 gdb @dots{}
 @end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use. 
+@end table
 
 @node Search
 @section Searching Source Files
diff -upr OLD/src/gdb/doc/refcard.tex NEW/src/gdb/doc/refcard.tex
--- OLD/src/gdb/doc/refcard.tex	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/refcard.tex	2009-01-19 08:57:19.000000000 -0200
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec
 
diff -upr OLD/src/gdb/utils.c NEW/src/gdb/utils.c
--- OLD/src/gdb/utils.c	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/utils.c	2009-01-19 09:25:19.000000000 -0200
@@ -96,6 +96,10 @@ static void prompt_for_continue (void);
 static void set_screen_size (void);
 static void set_width (void);
 
+/* External text editor */
+
+static char *external_editor_command = NULL;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 
 static int debug_timestamp = 0;
@@ -2694,6 +2698,12 @@ When set, debugging messages will be mar
 			   NULL,
 			   show_debug_timestamp,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+			    _("\
+Show the external text editor."), NULL,
+			    NULL, NULL,
+			    &setlist, &showlist);
 }
 
 /* Machine specific function to handle SIGWINCH signal. */
@@ -3443,3 +3453,16 @@ gdb_buildargv (const char *s)
     nomem (0);
   return argv;
 }
+
+/* Returns the external editor */
+char *
+external_editor (void)
+{
+  char *editor;
+  if (external_editor_command)
+    return external_editor_command;
+  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+    editor = "/bin/ex";
+  return editor;
+
+}

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 4300 bytes --]

2009/2/19 Alfredo Ortega <ortegaalfredo@gmail.com>:
> 2009/2/2 Tom Tromey <tromey@redhat.com>:
>>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:
>>
>> Alfredo> 1) Following the suggestion of Tom Tromey, i made "commands"
>> Alfredo> a prefix command, and now it is "commands edit n".
>>
>> Your patch does this using strncmp -- but gdb already has built-in
>> machinery for prefix commands.  See add_prefix_cmd.  So, the idea here
>> would be to change _initialize_breakpoint to use add_prefix_cmd,
>> instead of add_com, when creating the "commands" command.  Then, you'd
>> have a separate function to implement "commands edit".  Maybe this
>> means introducing a helper function to do some of the work, I don't
>> know.
>>
>> Alfredo> This is a much better patch, but also is a much bigger one (I already
>> Alfredo> sent the FSF form that Tom suggested), so surely there are plenty of
>> Alfredo> errors. Corrections are welcomed.
>>
>> A few nits inline.
>>
>> Alfredo> +#define COMMANDS_EDCOMMAND "edit"
>>
>> With the prefix change, you won't need this.
>>
>> Alfredo> +  /* Edit commands with external editor */
>>
>> In the GNU style, comments are full sentences that start with a
>> capital letter and end with a period and two spaces.  This one is
>> missing the period, but others are incorrect in other ways.
>>
>> Alfredo> +      /* discard the "edit" command */
>>
>> E.g., this one...
>>
>> Alfredo> +      get_number (&p);
>> Alfredo> +      bnum = get_number (&p);
>>
>> Two calls to get_number seems suspect.
>>
>> Alfredo> +      /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
>>
>> There's no need for this comment, IMO.
>>
>> Alfredo> +      if (!(vitmp = make_temp_file (NULL)))
>>
>> GNU style prohibits assignments in conditionals.
>>
>> Alfredo> +      {
>> Alfredo> +        error (_("Can't create temporary file for editing."));
>> Alfredo> +        return;
>> Alfredo> +      }
>>
>> The "error" function never returns.  It calls longjmp.  So, this
>> return is not needed.  This occurs in a few spots.
>>
>> Alfredo> +            l = b->commands;
>> Alfredo> +            while (l)
>> Alfredo> +              {
>> Alfredo> +                fsize = 0;
>> Alfredo> +                fsize += fwrite (l->line, 1, strlen (l->line), tmpstream);
>>
>> I think you should probably use "print_command_lines" to print the
>> breakpoint commands to a file.
>>
>> Alfredo> +        sysret = system (cmdline);
>> Alfredo> +        xfree (cmdline);
>> Alfredo> +        if (sysret < 0)
>>
>> I think this should also check "sysret" when it is >= 0, and fail if
>> the editor does not exit with status 0.
>>
>> Thanks for working on this,
>> Tom
>>
>
> Sorry for the late answer.
> I refactored the whole patch, is getting better...add_prefix_cmd()
> made me sweat! :) and print_command_lines() is a little tricky to use,
> so maybe i didn't get it right this time.
> Also I send the FSF paperwork and tried to do all your corrections in
> the following patch.
> Thanks to you for teaching and the patience!
>
> Regards,
>
> Alfred
>

Sorry, I forgot the changelog in the last e-mail. I'm resending the patch.


2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
  * breakpoint.c (commands_command,_initialize_breakpoint,
edit_command,check_executing_commands, commands_edit_command,
commands_dump_to_file):
  Add the 'edit' keyword to the 'commands' command to allow the
  use of an external editor to add or modify commands.
  Added an utility function to dump breakpoint commands to a file.
  Added an utility function to launch an external editor on breakpoint commands.
  Joined common checks in commands_command and edit_command.
  * utils.c,defs.h (external_editor,initialize_utils):
  Added an utility function to return the external text editor of the system.
  Added "set external-editor" and "show external-editor" commands to
set/show the external editor variable

2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
  * gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option, for editing commands with an
external editor. Also, added a brief description of the "set external-editor"
and "show external-editor" commands.

[-- Attachment #2: commands2.diff --]
[-- Type: text/x-diff, Size: 9932 bytes --]

diff -upr OLD/src/gdb/breakpoint.c NEW/src/gdb/breakpoint.c
--- OLD/src/gdb/breakpoint.c	2009-01-14 03:10:29.000000000 -0200
+++ NEW/src/gdb/breakpoint.c	2009-02-19 06:12:26.000000000 -0200
@@ -45,6 +45,7 @@
 #include "completer.h"
 #include "gdb.h"
 #include "ui-out.h"
+#include "cli-out.h"
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
 #include "block.h"
@@ -585,20 +586,116 @@ condition_command (char *arg, int from_t
   error (_("No breakpoint number %d."), bnum);
 }
 
+/* Dumps breakpoint commands to a file.  */
 static void
-commands_command (char *arg, int from_tty)
+commands_dump_to_file (char *filename, struct breakpoint *b)
+{
+  struct cleanup *cleanups;
+  struct ui_file *old, *outfile = gdb_fopen (filename, "w");
+  cleanups = make_cleanup_ui_file_delete (outfile);
+  old = cli_out_set_stream (uiout, outfile);
+  print_command_lines (uiout, b->commands, 4);
+  cli_out_set_stream (uiout, old);
+  do_cleanups (cleanups);
+
+}
+
+/* Launches the editor on the breakpoint command.  */
+char *
+commands_edit_command (int bnum)
 {
+  char *vitmp = NULL;
+  char *editor;
   struct breakpoint *b;
-  char *p;
-  int bnum;
-  struct command_line *l;
+  char *cmdline = NULL;
+  int sysret;
+  /* Generates the temporary file name.  */
+  vitmp = make_temp_file (NULL);
+  if (!vitmp)
+      error (_("Can't create temporary file for editing."));
+  editor = external_editor ();
+  if (!editor)
+      error (_("External editor not found."));
+  ALL_BREAKPOINTS (b) if (b->number == bnum)
+    {
+      if (&b->commands)
+	commands_dump_to_file (vitmp, b);
+      /* Edit the file.  */
+      cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
+      sprintf (cmdline, "%s \"%s\"", editor, vitmp);
+      sysret = system (cmdline);
+      xfree (cmdline);
+      if (sysret < 0)
+	  error (_("Can't execute external editor."));
+      if (sysret > 0)
+	  error (_("External editor returned non-zero status."));
+    }
+  return vitmp;
+}
+
 
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
 
+static void
+check_executing_commands ()
+{
   if (executing_breakpoint_commands)
-    error (_("Can't use the \"commands\" command among a breakpoint's commands."));
+    error (_
+	   ("Can't use the \"commands\" command among a breakpoint's commands."));
+}
+
+/* Like commands_command but using an external editor.  */
+static void
+edit_command (char *args, int from_tty)
+{
+  int bnum;
+  char *p;
+  char *vitmp;
+  struct breakpoint *b;
+  struct command_line *l;
+  FILE *tmpstream = NULL;
+
+  check_executing_commands ();
+  p = args;
+  if (!p)
+    error (_("No breakpoint number."));
+  bnum = get_number (&p);
+  if (p && *p)
+    error (_("Unexpected extra arguments following breakpoint number."));
+  /* Launches the editor.  */
+  vitmp = commands_edit_command (bnum);
+  if (!vitmp)
+    return;
+
+  ALL_BREAKPOINTS (b) if (b->number == bnum)
+    {
+      /* Redirect instream to the commands temporal file.  */
+      tmpstream = instream;
+      instream = fopen (vitmp, "r");
+      l = read_command_lines (NULL, from_tty, 1);
+      free_command_lines (&b->commands);
+      b->commands = l;
+      breakpoints_changed ();
+      observer_notify_breakpoint_modified (b->number);
+      /* Restore instream and erase temporal file.  */
+      instream = tmpstream;
+      unlink (vitmp);
+      return;
+    }
+  error (_("No breakpoint number %d."), bnum);
+}
+
+static void
+commands_command (char *arg, int from_tty)
+{
+  struct breakpoint *b;
+  char *p;
+  int bnum;
+  struct command_line *l;
+
+  check_executing_commands();
 
   p = arg;
   bnum = get_number (&p);
@@ -8052,6 +8149,12 @@ Multiple breakpoints at one place are pe
 \n\
 Do \"help breakpoints\" for info on other commands dealing with breakpoints."
 
+/* List of subcommands for "commands".  */
+static struct cmd_list_element *commands_command_list;
+
+/* List of subcommands for "edit".  */
+static struct cmd_list_element *edit_command_list;
+
 /* List of subcommands for "catch".  */
 static struct cmd_list_element *catch_cmdlist;
 
@@ -8100,14 +8203,20 @@ Usage is `ignore N COUNT'."));
   if (xdb_commands)
     add_com_alias ("bc", "ignore", class_breakpoint, 1);
 
-  add_com ("commands", class_breakpoint, commands_command, _("\
+  add_prefix_cmd ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
 Give \"silent\" as the first line to make the breakpoint silent;\n\
-then no output is printed when it is hit, except what the commands print."));
+then no output is printed when it is hit, except what the commands print."),
+		      &commands_command_list, "commands ", 1, &cmdlist);
+
+  add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\
+Edit the command using the external-editor variable, EDITOR environment\n\
+variable or /bin/ex, in that precedence."),
+		      &edit_command_list, "command edit ", 1, &commands_command_list);
 
   add_com ("condition", class_breakpoint, condition_command, _("\
 Specify breakpoint number N to break only if COND is true.\n\
diff -upr OLD/src/gdb/defs.h NEW/src/gdb/defs.h
--- OLD/src/gdb/defs.h	2009-01-14 03:10:28.000000000 -0200
+++ NEW/src/gdb/defs.h	2009-01-19 08:57:04.000000000 -0200
@@ -330,6 +330,8 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
+extern char *external_editor( void );
+
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
diff -upr OLD/src/gdb/doc/gdb.texinfo NEW/src/gdb/doc/gdb.texinfo
--- OLD/src/gdb/doc/gdb.texinfo	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/gdb.texinfo	2009-01-19 09:26:37.000000000 -0200
@@ -3976,6 +3976,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5384,6 +5386,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5587,6 +5590,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
@@ -5611,6 +5615,19 @@ or in the @code{csh} shell,
 setenv EDITOR /usr/bin/vi
 gdb @dots{}
 @end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use. 
+@end table
 
 @node Search
 @section Searching Source Files
diff -upr OLD/src/gdb/doc/refcard.tex NEW/src/gdb/doc/refcard.tex
--- OLD/src/gdb/doc/refcard.tex	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/doc/refcard.tex	2009-01-19 08:57:19.000000000 -0200
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec
 
diff -upr OLD/src/gdb/utils.c NEW/src/gdb/utils.c
--- OLD/src/gdb/utils.c	2009-01-14 03:10:27.000000000 -0200
+++ NEW/src/gdb/utils.c	2009-01-19 09:25:19.000000000 -0200
@@ -96,6 +96,10 @@ static void prompt_for_continue (void);
 static void set_screen_size (void);
 static void set_width (void);
 
+/* External text editor */
+
+static char *external_editor_command = NULL;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 
 static int debug_timestamp = 0;
@@ -2694,6 +2698,12 @@ When set, debugging messages will be mar
 			   NULL,
 			   show_debug_timestamp,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+			    _("\
+Show the external text editor."), NULL,
+			    NULL, NULL,
+			    &setlist, &showlist);
 }
 
 /* Machine specific function to handle SIGWINCH signal. */
@@ -3443,3 +3453,16 @@ gdb_buildargv (const char *s)
     nomem (0);
   return argv;
 }
+
+/* Returns the external editor */
+char *
+external_editor (void)
+{
+  char *editor;
+  if (external_editor_command)
+    return external_editor_command;
+  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+    editor = "/bin/ex";
+  return editor;
+
+}

Re: Use external editor in 'commands' command

[Tom Tromey]

>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:

Alfredo> 2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
Alfredo>   * breakpoint.c (commands_command,_initialize_breakpoint,
Alfredo> edit_command,check_executing_commands, commands_edit_command,
Alfredo> commands_dump_to_file):
Alfredo>   Add the 'edit' keyword to the 'commands' command to allow the
Alfredo>   use of an external editor to add or modify commands.
Alfredo>   Added an utility function to dump breakpoint commands to a file.
Alfredo>   Added an utility function to launch an external editor on breakpoint commands.
Alfredo>   Joined common checks in commands_command and edit_command.

This is wordier than the norm.  Also, a comment describing a change
should be next to the name of the function it describes.  This seems
to list all the function names, then all the changes.

See the existing ChangeLog for a lot of examples.  Also the GNU
standards have a section on writing a ChangeLog entry...

Alfredo> +commands_dump_to_file (char *filename, struct breakpoint *b)
Alfredo> +{
Alfredo> +  struct cleanup *cleanups;
Alfredo> +  struct ui_file *old, *outfile = gdb_fopen (filename, "w");

You have to check for a NULL return from gdb_fopen.

Alfredo> +  do_cleanups (cleanups);
Alfredo> +
Alfredo> +}

Extra blank line in there.

Alfredo> +/* Launches the editor on the breakpoint command.  */
Alfredo> +char *
Alfredo> +commands_edit_command (int bnum)

The header comment should describe the meaning of the return value and
the arguments.  For a string return, like this, it should also
describe how the memory is managed.

Alfredo> +  if (!vitmp)
Alfredo> +      error (_("Can't create temporary file for editing."));

Two extra spaces of indentation on the second line, not four.  This
occurs in a few spots.

Alfredo> +  ALL_BREAKPOINTS (b) if (b->number == bnum)

The 'if' should go on a new line.
This will require some reindentation, as well.

Alfredo> +      cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);

Instead of 50, just use the correct number.
Or, use xstrprintf, which is simpler.

Alfredo> +      if (sysret < 0)
Alfredo> +	  error (_("Can't execute external editor."));
Alfredo> +      if (sysret > 0)
Alfredo> +	  error (_("External editor returned non-zero status."));

I think you need to use WEXITSTATUS and friends here.

Alfredo> +  ALL_BREAKPOINTS (b) if (b->number == bnum)

Newline.

Alfredo> +    {
Alfredo> +      /* Redirect instream to the commands temporal file.  */

I think this can be just "Read commands from the temporary file."

Alfredo> +      instream = fopen (vitmp, "r");

Needs error checking.

Alfredo> +      unlink (vitmp);

I suspect this should probably be done via a cleanup.

Alfredo> +/* List of subcommands for "edit".  */
Alfredo> +static struct cmd_list_element *edit_command_list;

I don't think you need this...

Alfredo> +  add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\

... because I don't think "edit" needs to be a prefix command.
It can just be an ordinary command.

Alfredo> +extern char *external_editor( void );

Should be " (void)", not "( void )".

Alfredo> +  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\

I don't remember... did we already discuss the name of the option?
Why "external-editor" and not the simpler "editor"?

Alfredo> +/* Returns the external editor */

Needs period and 2 spaces at the end.

Alfredo> +  return editor;
Alfredo> +
Alfredo> +}

Extra blank line.

This is pretty close to ready.  Thanks for persevering.

Tom

Re: Use external editor in 'commands' command

[Alfredo Ortega]

[-- Attachment #1: Type: text/plain, Size: 4991 bytes --]

Hi all,

Sorry Tom for the very late answer. But I think I got all your
recommendations implemented on the patch.
Except that I couldn't find a way to do a cleanup erasing a file,
without resorting to the ui_file functions.
But that wouldn't work because I need a *FILE for instream. Any hint?
Sending my latest patch anyway if someone want to take a look at it.
Regards,

Alfred


2009-08-28 Alfredo Ortega <ortegaalfredo@gmail.com>
 * breakpoint.c (commands_command,_initialize_breakpoint,
edit_command,check_executing_commands, commands_edit_command,
commands_dump_to_file):
Add the 'edit' keyword to the 'commands' command.
 * utils.c,defs.h (external_editor,initialize_utils):
 Added an utility function to return the external text editor of the system.
 Added new "set external-editor" and "show external-editor" commands.

2009-08-28 Alfredo Ortega <ortegaalfredo@gmail.com>
 * gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option for editing commands.



2009/2/24 Tom Tromey <tromey@redhat.com>:
>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:
>
> Alfredo> 2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
> Alfredo>   * breakpoint.c (commands_command,_initialize_breakpoint,
> Alfredo> edit_command,check_executing_commands, commands_edit_command,
> Alfredo> commands_dump_to_file):
> Alfredo>   Add the 'edit' keyword to the 'commands' command to allow the
> Alfredo>   use of an external editor to add or modify commands.
> Alfredo>   Added an utility function to dump breakpoint commands to a file.
> Alfredo>   Added an utility function to launch an external editor on breakpoint commands.
> Alfredo>   Joined common checks in commands_command and edit_command.
>
> This is wordier than the norm.  Also, a comment describing a change
> should be next to the name of the function it describes.  This seems
> to list all the function names, then all the changes.
>
> See the existing ChangeLog for a lot of examples.  Also the GNU
> standards have a section on writing a ChangeLog entry...
>
> Alfredo> +commands_dump_to_file (char *filename, struct breakpoint *b)
> Alfredo> +{
> Alfredo> +  struct cleanup *cleanups;
> Alfredo> +  struct ui_file *old, *outfile = gdb_fopen (filename, "w");
>
> You have to check for a NULL return from gdb_fopen.
>
> Alfredo> +  do_cleanups (cleanups);
> Alfredo> +
> Alfredo> +}
>
> Extra blank line in there.
>
> Alfredo> +/* Launches the editor on the breakpoint command.  */
> Alfredo> +char *
> Alfredo> +commands_edit_command (int bnum)
>
> The header comment should describe the meaning of the return value and
> the arguments.  For a string return, like this, it should also
> describe how the memory is managed.
>
> Alfredo> +  if (!vitmp)
> Alfredo> +      error (_("Can't create temporary file for editing."));
>
> Two extra spaces of indentation on the second line, not four.  This
> occurs in a few spots.
>
> Alfredo> +  ALL_BREAKPOINTS (b) if (b->number == bnum)
>
> The 'if' should go on a new line.
> This will require some reindentation, as well.
>
> Alfredo> +      cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
>
> Instead of 50, just use the correct number.
> Or, use xstrprintf, which is simpler.
>
> Alfredo> +      if (sysret < 0)
> Alfredo> +        error (_("Can't execute external editor."));
> Alfredo> +      if (sysret > 0)
> Alfredo> +        error (_("External editor returned non-zero status."));
>
> I think you need to use WEXITSTATUS and friends here.
>
> Alfredo> +  ALL_BREAKPOINTS (b) if (b->number == bnum)
>
> Newline.
>
> Alfredo> +    {
> Alfredo> +      /* Redirect instream to the commands temporal file.  */
>
> I think this can be just "Read commands from the temporary file."
>
> Alfredo> +      instream = fopen (vitmp, "r");
>
> Needs error checking.
>
> Alfredo> +      unlink (vitmp);
>
> I suspect this should probably be done via a cleanup.
>
> Alfredo> +/* List of subcommands for "edit".  */
> Alfredo> +static struct cmd_list_element *edit_command_list;
>
> I don't think you need this...
>
> Alfredo> +  add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\
>
> ... because I don't think "edit" needs to be a prefix command.
> It can just be an ordinary command.
>
> Alfredo> +extern char *external_editor( void );
>
> Should be " (void)", not "( void )".
>
> Alfredo> +  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
>
> I don't remember... did we already discuss the name of the option?
> Why "external-editor" and not the simpler "editor"?
>
> Alfredo> +/* Returns the external editor */
>
> Needs period and 2 spaces at the end.
>
> Alfredo> +  return editor;
> Alfredo> +
> Alfredo> +}
>
> Extra blank line.
>
> This is pretty close to ready.  Thanks for persevering.
>
> Tom
>

[-- Attachment #2: commands.patch --]
[-- Type: text/x-diff, Size: 10639 bytes --]

diff -u -p -r1.418 breakpoint.c
--- gdb/breakpoint.c	21 Aug 2009 18:54:44 -0000	1.418
+++ gdb/breakpoint.c	28 Aug 2009 22:11:05 -0000
@@ -47,6 +47,7 @@
 #include "completer.h"
 #include "gdb.h"
 #include "ui-out.h"
+#include "cli-out.h"
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
 #include "block.h"
@@ -652,20 +653,129 @@ breakpoint_set_commands (struct breakpoi
   observer_notify_breakpoint_modified (b->number);
 }
 
+/* Dumps breakpoint commands to a file.  */
 static void
-commands_command (char *arg, int from_tty)
+commands_dump_to_file (char *filename, struct breakpoint *b)
 {
+  struct cleanup *cleanups;
+  struct ui_file *old, *outfile = gdb_fopen (filename, "w");
+  if (outfile==NULL)
+      error (_("Can't create temporary file for editing."));
+  cleanups = make_cleanup_ui_file_delete (outfile);
+  old = cli_out_set_stream (uiout, outfile);
+  print_command_lines (uiout, b->commands, 4);
+  cli_out_set_stream (uiout, old);
+  do_cleanups (cleanups);
+}
+
+/* Launches the editor on the breakpoint command.  
+   bnum is the breakpoint number.
+   Returns the name of a temporary file containing
+   the edited commands. The returned string must be freed. */
+char *
+commands_edit_command (int bnum)
+{
+  char *vitmp = NULL;
+  char *editor;
   struct breakpoint *b;
-  char *p;
-  int bnum;
-  struct command_line *l;
+  char *cmdline = NULL;
+  int sysret;
+  /* Generates the temporary file name.  */
+  vitmp = make_temp_file (NULL);
+  if (!vitmp)
+    error (_("Can't create temporary file for editing."));
+  editor = external_editor ();
+  if (!editor)
+    error (_("External editor not found."));
+  ALL_BREAKPOINTS (b)
+    if (b->number == bnum)
+      {
+        if (&b->commands)
+	  commands_dump_to_file (vitmp, b);
+        /* Edit the file.  */
+	cmdline = xstrprintf("%s \"%s\"",editor,vitmp);
+        sysret = system (cmdline);
+        xfree (cmdline);
+        if (sysret < 0)
+  	  error (_("Can't execute external editor."));
+        if (WEXITSTATUS(sysret) > 0)
+  	  error (_("External editor returned non-zero status."));
+      }
+  return vitmp;
+}
 
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
 
+static void
+check_executing_commands ()
+{
   if (executing_breakpoint_commands)
     error (_("Can't use the \"commands\" command among a breakpoint's commands."));
+}
+
+/* Like commands_command but using an external editor.  */
+static void
+edit_command (char *args, int from_tty)
+{
+  int bnum;
+  char *p;
+  char *vitmp;
+  struct breakpoint *b;
+  struct command_line *l;
+  FILE *tmpstream = NULL;
+  struct cleanup *cleanups;
+
+  check_executing_commands ();
+  p = args;
+  if (!p)
+    error (_("No breakpoint number."));
+  bnum = get_number (&p);
+  if (p && *p)
+    error (_("Unexpected extra arguments following breakpoint number."));
+  /* Launches the editor.  */
+  vitmp = commands_edit_command (bnum);
+  if (!vitmp)
+    error (_("Can't create temporary file for editing."));
+  cleanups = make_cleanup(xfree,vitmp);
+
+  ALL_BREAKPOINTS (b)
+    if (b->number == bnum)
+      {
+        /* Read commands from the temporary file.  */
+        tmpstream = instream;
+        instream = fopen (vitmp, "r");
+	if (instream==NULL)
+	  {
+	  instream=tmpstream;
+    	  error (_("Can't open commands temporary file."));
+	  }
+        l = read_command_lines (NULL, from_tty, 1);
+        free_command_lines (&b->commands);
+        b->commands = l;
+        breakpoints_changed ();
+        observer_notify_breakpoint_modified (b->number);
+        /* Restore instream and erase temporal file.  */
+        instream = tmpstream;
+	unlink(vitmp);
+  	do_cleanups (cleanups);
+        return;
+      }
+  unlink(vitmp);
+  do_cleanups (cleanups);
+  error (_("No breakpoint number %d."), bnum);
+}
+
+static void
+commands_command (char *arg, int from_tty)
+{
+  struct breakpoint *b;
+  char *p;
+  int bnum;
+  struct command_line *l;
+
+  check_executing_commands();
 
   p = arg;
   bnum = get_number (&p);
@@ -8891,6 +9001,9 @@ Multiple breakpoints at one place are pe
 \n\
 Do \"help breakpoints\" for info on other commands dealing with breakpoints."
 
+/* List of subcommands for "commands".  */
+static struct cmd_list_element *commands_command_list;
+
 /* List of subcommands for "catch".  */
 static struct cmd_list_element *catch_cmdlist;
 
@@ -8941,14 +9054,20 @@ Usage is `ignore N COUNT'."));
   if (xdb_commands)
     add_com_alias ("bc", "ignore", class_breakpoint, 1);
 
-  add_com ("commands", class_breakpoint, commands_command, _("\
+  add_prefix_cmd ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
 Give \"silent\" as the first line to make the breakpoint silent;\n\
-then no output is printed when it is hit, except what the commands print."));
+then no output is printed when it is hit, except what the commands print."),
+		      &commands_command_list, "commands ", 1, &cmdlist);
+
+  add_cmd ("edit", class_breakpoint, edit_command,_("\
+Edit the command using the external-editor variable, EDITOR environment\n\
+variable or /bin/ex, in that precedence."),
+		      &commands_command_list);
 
   add_com ("condition", class_breakpoint, condition_command, _("\
 Specify breakpoint number N to break only if COND is true.\n\
Index: gdb/defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.254
diff -u -p -r1.254 defs.h
--- gdb/defs.h	3 Aug 2009 12:26:37 -0000	1.254
+++ gdb/defs.h	28 Aug 2009 22:11:06 -0000
@@ -335,6 +335,8 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
+extern char *external_editor(void);
+
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
Index: gdb/utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.219
diff -u -p -r1.219 utils.c
--- gdb/utils.c	18 Aug 2009 16:17:16 -0000	1.219
+++ gdb/utils.c	28 Aug 2009 22:11:07 -0000
@@ -98,6 +98,10 @@ static void prompt_for_continue (void);
 static void set_screen_size (void);
 static void set_width (void);
 
+/* External text editor */
+
+static char *external_editor_command = NULL;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 
 static int debug_timestamp = 0;
@@ -2834,6 +2838,12 @@ When set, debugging messages will be mar
 			   NULL,
 			   show_debug_timestamp,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+			    _("\
+Show the external text editor."), NULL,
+			    NULL, NULL,
+			    &setlist, &showlist);
 }
 
 /* Machine specific function to handle SIGWINCH signal. */
@@ -3563,3 +3573,15 @@ _initialize_utils (void)
   add_internal_problem_command (&internal_error_problem);
   add_internal_problem_command (&internal_warning_problem);
 }
+
+/* Returns the external editor.  */
+char *
+external_editor (void)
+{
+  char *editor;
+  if (external_editor_command)
+    return external_editor_command;
+  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+    editor = "/bin/ex";
+  return editor;
+}
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.618
diff -u -p -r1.618 gdb.texinfo
--- gdb/doc/gdb.texinfo	27 Aug 2009 21:56:38 -0000	1.618
+++ gdb/doc/gdb.texinfo	28 Aug 2009 22:11:17 -0000
@@ -4024,6 +4024,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5675,6 +5677,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5878,6 +5881,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
@@ -5902,6 +5906,19 @@ or in the @code{csh} shell,
 setenv EDITOR /usr/bin/vi
 gdb @dots{}
 @end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use. 
+@end table
 
 @node Search
 @section Searching Source Files
Index: gdb/doc/refcard.tex
===================================================================
RCS file: /cvs/src/src/gdb/doc/refcard.tex,v
retrieving revision 1.6
diff -u -p -r1.6 refcard.tex
--- gdb/doc/refcard.tex	27 Mar 2007 18:09:35 -0000	1.6
+++ gdb/doc/refcard.tex	28 Aug 2009 22:11:17 -0000
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec
 

Re: Use external editor in 'commands' command

[Tom Tromey]

>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:

Alfredo> Sorry Tom for the very late answer. But I think I got all your
Alfredo> recommendations implemented on the patch.

Thanks.  And, I'm also sorry for the delay in this response :-)

Alfredo> Except that I couldn't find a way to do a cleanup erasing a file,
Alfredo> without resorting to the ui_file functions.

You can make a new handler function and directly call make_cleanup.
Search for make_cleanup, there are plenty of examples of this.

Alfredo> +  if (outfile==NULL)
Alfredo> +      error (_("Can't create temporary file for editing."));

The if should be written:  if (outfile == NULL).
The GNU style puts spaces around operators.

Also the second line is indented too far.

Alfredo> +/* Launches the editor on the breakpoint command.  
Alfredo> +   bnum is the breakpoint number.

Write "BNUM" instead.  See the coding styles for the reason.

Alfredo> +  ALL_BREAKPOINTS (b)
Alfredo> +    if (b->number == bnum)
Alfredo> +      {
Alfredo> +        if (&b->commands)

This check is wrong.  &b->commands will never be NULL.
You meant just 'if (b->commands)'.

Alfredo> +	  commands_dump_to_file (vitmp, b);
Alfredo> +        /* Edit the file.  */
Alfredo> +	cmdline = xstrprintf("%s \"%s\"",editor,vitmp);

Indentation is weird.  I guess you are mixing tabs and spaces.
Exactly which to use is often discussed; but being inconsistent is worse
than just picking one :-)

Alfredo> +        if (WEXITSTATUS(sysret) > 0)

Add a space before the "(".  This problem occurs in several places,
please fix them all.  (But this rule does not apply to "_("... fun :-)

Alfredo> +static void
Alfredo> +check_executing_commands ()

Use (void), not ().

Alfredo> +/* Like commands_command but using an external editor.  */
Alfredo> +static void
Alfredo> +edit_command (char *args, int from_tty)

Rather than duplicate a lot of commands_command, I think it would be
preferable to refactor this: rename commands_command, add an argument
indicating whether an editor should be used, and then have the new
commands_command and edit_command call the internal function.

Then you don't need check_executing_commands, either.

Alfredo> +        b->commands = l;
Alfredo> +        breakpoints_changed ();
Alfredo> +        observer_notify_breakpoint_modified (b->number);

Also, update to CVS head.  I think we're using breakpoint_set_commands now.

Alfredo> +  add_cmd ("edit", class_breakpoint, edit_command,_("\
Alfredo> +Edit the command using the external-editor variable, EDITOR environment\n\
Alfredo> +variable or /bin/ex, in that precedence."),

I think this needs a bit more documentation.  Maybe:

Like `commands', but edit the commands using an editor.
If there is an external editor (see `help set external-editor'), then
that is used.  Otherwise, if the EDITOR environment variable is set,
then it is used.  If all else fails, `/bin/ex' is used.

Alfredo> +@table @code
Alfredo> +@item set external-editor
Alfredo> +@kindex set external-editor
Alfredo> +This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.

This at least needs some line breaks.

Tom