Re: Use external editor in 'commands' command

Eli Zaretskii
2009-01-16T09:07:17Z

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