This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Use external editor in 'commands' command
> 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.