[prev in list] [next in list] [prev in thread] [next in thread]
List: pgsql-hackers
Subject: Re: [HACKERS] Patch for checking file parameters to psql before password prompt
From: Stephen Frost <sfrost () snowman ! net>
Date: 2012-12-29 20:36:28
Message-ID: 20121229203628.GD16126 () tamriel ! snowman ! net
[Download RAW message or body]
Alastair,
* Alastair Turner (bell@ctrlf5.co.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
["signature.asc" (application/pgp-signature)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic