Re: Patch for checking file parameters to psql before password prompt

Alastair,

* Alastair Turner (bell(at)ctrlf5(dot)co(dot)za) wrote:
> Patch for the changes discussed in
> http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php
> attached (eventually ...)
>
> In summary: If the input file (-f) doesn't exist or the ouput or log
> files (-o and -l) can't be created psql exits before prompting for a
> password.

On a once-over, the patch looks reasonably good. A couple things
though:

Please be good with the whitespace:

Above 'if (options.logfilename)' you introduce an extra \n, while you
don't have one between the end of that if() { } block and the next. You
also have a whole diff block that's just adding a \n (between
"pset.inputfile = oldfilename;" and "return result;"). Reviewing your
patches before sending them is a good way to find these things. :)

Silly stuff, sure, but since it's your first patch, I figured I'd
mention it. :)

Also, if you're doing the error-reporting in get_fd_for_process and then
every time it's called and returns failure immediately exiting, why not
just error-exit from get_fd_for_process directly..?

Lastly, I'm not convinced that how you broke up process_file() and
process_fd() actually works. Inside the existing process_file(),
filename will be updated to '<stdin>' for error reporting when the input
in 'stdin', but that's now lost in the new process_file() and
process_fd() will always get whatever is in options.action_string, which
could be a '-' instead. In reviewing the patch, I was hoping that
process_fd() wouldn't actually need to have the filename passed in with
the fd, but it does because psql_error() depends on pset.inputfile being
set, which has to be done by the code which calls into MainLoop(), which
is process_fd() with your patch.

Perhaps there's a better way to handle that?

Thanks,

Stephen