Re: pgwin32_open returning EINVAL

Magnus Hagander wrote:
> On Thu, Nov 29, 2007 at 09:09:47AM -0300, Alvaro Herrera wrote:
> > Magnus Hagander wrote:
> > > On Wed, Nov 28, 2007 at 05:20:46PM +0100, Magnus Hagander wrote:
> > > > On Wed, Nov 28, 2007 at 12:24:26PM -0300, Alvaro Herrera wrote:
> > > > > Martijn van Oosterhout wrote:
> > > > > > On Wed, Nov 28, 2007 at 11:57:35AM -0300, Alvaro Herrera wrote:
> > > > > > > Can we do something like this to report the Win32 error code so that the
> > > > > > > user has a higher chance of figuring out what's going on?
> > > >
> > > > We already do something very similar to what you're donig in
> > > > backend/port/wni32/socket.c :: TranslateSocketError().
> > > >
> > > > So I think it's a good idea to do it, yes.
> > >
> > > Thinking about this some more, I think this is a better patch - we already
> > > have a function that takes care of this, including both error and debug
> > > logging. Anybody disagree? If not, I'll go ahead and apply it...
> >
> > Hmm, but this still mixes some error codes. To absolutely get the
> > Windows error code you would have to be running with DEBUG5, which I
> > don't think is acceptable for a production system during any interesting
> > length of time ... Can we have that DEBUG5 message changed to LOG
> > instead?
>
> Maybe. I'm concerned we might end up logging a whole lot more, for cases
> where it's not an actual error. For example, a file that doesn't exist
> doesn't necessarily mean it's an error... I don't want to have to go
> through all code-paths that end up calling that function to see if that may
> be so...

I just checked. I see there are only five callers. In three cases (two
in file/fd.c and one in port/dirent.c), there is at a single error code
which is "possibly expected". It is taken care of without calling
_dosmaperr at all. In syslogger.c there are two possibly expected error
codes, dealt with in the same way. And the last caller is
port/getrusage.c, which has no possibly-expected error code.

So I don't think this is a concern -- whenever _dosmaperr is called, a
"true" error message is already going to be logged.

(The only case where a message would be logged inappropriately would be
in file/fd.c if _dosmaperr returned EINTR, but AFAICS we don't map any
code to that).

> But it is true that we have mixed error codes. But we only do that when we
> know they're actually there. If we have an unknown code, we don't just
> return it as EINVAL without logging (as open did before) - and that log
> goes out as LOG.

Yeah, I understand. But for example there are several different cases
that are mapped to EACCES. In the cases we're currently following,
having the exact error code could prove crucial to determining the cause
of the error.

> (the open code already mixed two win32 codes into ENOENT already, btw)

Yeah, I saw that, but it's not really a problem because the errors are
"file does not exist" and "dir does not exist", and it can be very
easily confirmed whether the file and dir actually exist or not. But if
you have ERROR_LOCK_VIOLATION reported identically to
ERROR_SHARING_VIOLATION, or whatever, there's no way you can tell which
one actually occured.

> > Or maybe have _dosmaperr receive the elevel with which to report the
> > message as a parameter, and have current callers use DEBUG5, and
> > pgwin32_open use LOG. That way we can backpatch it to 8.2 without
> > changing current behavior.
>
> I don't think we can, or at least should, touch the signature for
> _dosmaperr(). It's a system replacment, I think we should keep the same
> signature for it.

Good point. If this were truly a problem we could make another routine
with the same code called differently, but I don't think it's really
important.

--
Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)