Обсуждение: Sloppy SSPI error reporting code
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.
I have no intention of fixing these myself, but whoever committed that
code should take a second look.
regards, tom lane
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote: > While looking at fe-auth.c I noticed quite a few places that weren't > bothering to make error messages localizable (ie, missing libpq_gettext > calls), and/or were failing to add a trailing newline as expected in > libpq error messages. Perhaps these are intentional but I doubt it. > Most of the instances seemed to be SSPI-related. > > I have no intention of fixing these myself, but whoever committed that > code should take a second look. I looked through that file and only found two cases; patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Вложения
On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
> On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
> > While looking at fe-auth.c I noticed quite a few places that weren't
> > bothering to make error messages localizable (ie, missing libpq_gettext
> > calls), and/or were failing to add a trailing newline as expected in
> > libpq error messages. Perhaps these are intentional but I doubt it.
> > Most of the instances seemed to be SSPI-related.
> >
> > I have no intention of fixing these myself, but whoever committed that
> > code should take a second look.
>
> I looked through that file and only found two cases; patch attached.
Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
> ! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSPI returned invalid number of output
buffers\n"));
> ! libpq_gettext("fe_sendauth: error sending password authentication\n"));
The first message is too internals-focused for a translation to be useful. If
it were in the backend, we would use elog() and not translate. The second is
a non-actionable message painting over a wide range of specific failures;
translating it would just put lipstick on the pig.
On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
> On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
> > On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
> > > While looking at fe-auth.c I noticed quite a few places that weren't
> > > bothering to make error messages localizable (ie, missing libpq_gettext
> > > calls), and/or were failing to add a trailing newline as expected in
> > > libpq error messages. Perhaps these are intentional but I doubt it.
> > > Most of the instances seemed to be SSPI-related.
> > >
> > > I have no intention of fixing these myself, but whoever committed that
> > > code should take a second look.
> >
> > I looked through that file and only found two cases; patch attached.
>
> Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
Oh, I accidentally saw the backend version of that function, which
looked fine. I have attached a patch for that.
> > ! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSPI returned invalid number of output
buffers\n"));
> > ! libpq_gettext("fe_sendauth: error sending password authentication\n"));
>
> The first message is too internals-focused for a translation to be useful. If
> it were in the backend, we would use elog() and not translate. The second is
> a non-actionable message painting over a wide range of specific failures;
> translating it would just put lipstick on the pig.
OK.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote: > On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote: > > On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote: > > > On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote: > > > > While looking at fe-auth.c I noticed quite a few places that weren't > > > > bothering to make error messages localizable (ie, missing libpq_gettext > > > > calls), and/or were failing to add a trailing newline as expected in > > > > libpq error messages. Perhaps these are intentional but I doubt it. > > > > Most of the instances seemed to be SSPI-related. > > > > > > > > I have no intention of fixing these myself, but whoever committed that > > > > code should take a second look. > > > > > > I looked through that file and only found two cases; patch attached. > > > > Tom mentioned newline omissions, which you'll find in pg_SSPI_error(). > > Oh, I accidentally saw the backend version of that function, which > looked fine. I have attached a patch for that. That patch looks reasonable.
On Fri, Apr 3, 2015 at 08:32:08PM -0400, Noah Misch wrote: > On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote: > > On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote: > > > On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote: > > > > On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote: > > > > > While looking at fe-auth.c I noticed quite a few places that weren't > > > > > bothering to make error messages localizable (ie, missing libpq_gettext > > > > > calls), and/or were failing to add a trailing newline as expected in > > > > > libpq error messages. Perhaps these are intentional but I doubt it. > > > > > Most of the instances seemed to be SSPI-related. > > > > > > > > > > I have no intention of fixing these myself, but whoever committed that > > > > > code should take a second look. > > > > > > > > I looked through that file and only found two cases; patch attached. > > > > > > Tom mentioned newline omissions, which you'll find in pg_SSPI_error(). > > > > Oh, I accidentally saw the backend version of that function, which > > looked fine. I have attached a patch for that. > > That patch looks reasonable. Patch applied. Thanks. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +