Re: Use external editor in 'commands' command

[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.