Alright , I'll revert to PGPASS check.
Existing function only creates folder containing file. With this case, whats
expected ? Reading value in PGPASSFILE and try to create folder containing
pgpass file (Assuming it's valid path)? Remember, it's environment variable.
User can specify anything in there. Some garbage value as well. If we don't do
any validation there, user will automatically see error with complain about
file ?
thanks and regards,
Prasad
Sent: Wednesday, March 04, 2015 at 7:48 AM
From: "Ashesh Vashi" <ashesh.va...@enterprisedb.com>
To: Prasad <prasa...@mail.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
On Wed, Mar 4, 2015 at 8:44 AM, Prasad <prasa...@mail.com> wrote:
Ashesh,
Thanks for reviewing patch,
Code I have removed in I think, was switch statement inside if condition, which
doesn't make sense.
ie.
if (var == 2)
{
switch (var)
case 2:
.....
break;
}
that's why I removed it, because it's redundant.
Agree about redundancy, but you've also removed the code for checking the
PGPASS check at the start of the function.
i.e.
@@ -762,35 +762,33 @@ void sysSettings::SetCanonicalLanguage(const wxLanguage
&lang)
//////////////////////////////////////////////////////////////////////////
wxString sysSettings::GetConfigFile(configFileName cfgname)
{
- if (cfgname == PGPASS)
- {
I am not agree with that.
About creation of directory, I'm not sure if this validation is required.
Existing code creates directory postgresql (only on windows) according to
http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html[http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html]
, and it doesn't create file. I'm not sure whether this kind of validation is
expected in this function.
I think - it is.
Because - it could be used to save the updated password in the PGPASS file.
-- Ashesh
regards,
Prasad
Sent: Wednesday, March 04, 2015 at 7:15 AM
From: "Ashesh Vashi"
<ashesh.va...@enterprisedb.com[ashesh.va...@enterprisedb.com]>
To: Prasad <prasa...@mail.com[prasa...@mail.com]>
Cc: pgadmin-hackers
<pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]>
Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
Hi Prasad,
I see couple of issues with your patch.* Please generate the patch using 'git
diff'.
I could not apply your patch straight forwardly.
I had to use the patch utility.
* Please follow the coding style of pgAdmin.
You can find it at
https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*[https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*]
Do not remove any of the existing code.
It has been kept there keeping in mind about future development extending
support of the existing functionality.
You've removed couple of lines in the sysSettings::GetConfigFile(...)
function, which is not good.
In your code:* Checked only for PGPASSFILE environment variable.
* Need to check the existence of the file.
* Take required actions (if that file/parent directory does not exists).
i.e. Create parent directory
--
Thanks & Regards,
Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL
Company[http://www.enterprisedb.com[http://www.enterprisedb.com]]
http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi][http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]]
On Sun, Mar 1, 2015 at 11:08 PM, Prasad
<prasa...@mail.com[prasa...@mail.com][prasa...@mail.com[prasa...@mail.com]]>
wrote:
Hi,
Find attached fix for reading PGPASSFILE environment variable for pg password
file.
regards,
Prasad
--
Sent via pgadmin-hackers mailing list
(pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers][http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]]
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
