Issue 8192 - liblmdb confuses Windows error code with errno values
Summary: liblmdb confuses Windows error code with errno values
Status: UNCONFIRMED
Alias: None
Product: LMDB
Classification: Unclassified
Component: liblmdb (show other issues)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: OpenLDAP project
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-08 18:29 UTC by Hallvard Furuseth
Modified: 2024-04-02 17:50 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Hallvard Furuseth 2015-07-08 18:29:53 UTC
Full_Name: Hallvard B Furuseth
Version: LMDB_0.9.15
OS: Windows 7 with cygwin
URL: 
Submission from: (NULL) (81.191.45.5)
Submitted by: hallvard


mdb_env_write_meta() loops to retry_write if ErrCode() == EINTR.
That's ERROR_TOO_MANY_OPEN_FILES = 4 on Windows.  New in 0.9.15.

==

This one is older, in mdb_env_setup_locks().  Also wrong on Windows:
  rc = ErrCode();
  if (rc && rc != EACCES && rc != EAGAIN)

I don't know what that code is doing. I think that belongs under
  if ((rc = mdb_env_excl_lock(env, excl))) goto fail;
where it would read as
  if (*excl <= 0 && (rc = ErrCode()) && rc != EACCES && rc != EAGAIN)

==D%D

ENOENT confusion:

mdb_env_read_header() can explicitly return ENOENT, but only
as an internal code.  So mdb_strerror() should drop ENOENT.
We can rename/renumber it to avoid confusion:
  /* Internal result codes, not exposed outside liblmdb */
  #define	MDB_FILE_NEW	(MDB_LAST_ERRCODE + 9)
(above instead of MDB_NO_ROOT for easier cherry-picking.)

OTOH lmdb.h lists
 "ENOENT - the directory specified by the path parameter doesn't exist."
That's not related to the above.  It should be
"ENOENT (Unix) or ERROR_PATH_NOT_FOUND (Windows) - ..."

Or maybe the doc for this code should say something more
general, or simply be omitted.  I found the Windows code
by testing @ Windows.  Reading Windows doc did not help.

==

Finally, I suggest we replace the explicit EIO/ENOSPC returns
with either a new MDB code or #define MDB_EIO EIO on Unix
and something else on Windows.  (I think ENOSPC should be EIO
too, there could be other reasons for a short write.)
Then we can drop those from mdb_strerror() too.
Comment 1 Hallvard Furuseth 2015-07-08 20:24:56 UTC
On 08/07/15 20:29, h.b.furuseth@usit.uio.no wrote:
> Finally, I suggest we replace the explicit EIO/ENOSPC returns
> with either a new MDB code or #define MDB_EIO EIO on Unix
> and something else on Windows.

I mean, without waiting for LMDB-1.*.  They are fairly
obscure errors, I doubt anyone is testing for them.

Comment 2 Howard Chu 2015-07-08 20:36:41 UTC
h.b.furuseth@usit.uio.no wrote:
> On 08/07/15 20:29, h.b.furuseth@usit.uio.no wrote:
>> Finally, I suggest we replace the explicit EIO/ENOSPC returns
>> with either a new MDB code or #define MDB_EIO EIO on Unix
>> and something else on Windows.
>
> I mean, without waiting for LMDB-1.*.  They are fairly
> obscure errors, I doubt anyone is testing for them.

When there is a platform-specific error code that describes the situation, we 
should use it. In particular, if the error code comes from an underlying 
system call and accurately describes that failure, we should pass it through. 
If the failure is LMDB-specific, and not related to any particular system 
call, then we should define an MDB_* code for it.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Comment 3 Hallvard Furuseth 2015-07-08 21:09:47 UTC
Typo in original report:

 >   #define	MDB_FILE_NEW	(MDB_LAST_ERRCODE + 9)
 > (above instead of MDB_NO_ROOT for easier cherry-picking.)

"above instead of below MDB_NO_ROOT".


On 08/07/15 22:36, Howard Chu wrote:
> When there is a platform-specific error code that describes the
> situation, we should use it. In particular, if the error code comes from
> an underlying system call and accurately describes that failure, we
> should pass it through.

Yes.  I only meant to talk about where mdb.c explicitly picks
errno codes, like it does with EIO/ENOSPC.

(And where it compares codes from system calls with errno codes.)

> If the failure is LMDB-specific, and not related
> to any particular system call, then we should define an MDB_* code for it.