277870 - DBM Should use NSPR directly

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

Attachment: Patch v.1

Patch from Garrett's branch.

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 170885 [details] [diff] [review]
Patch v.1

Random Patch Comments...

These files are littered with \t's I would personally prefer to keep with that
style rather than changing *only* the lines in this patch, (would really prefer
someone to change the whole file[s], but thats another story for another bug)

>RCS file: /cvsroot/mozilla/dbm/include/hash.h,v
...
>+#if !defined(WINCE)
>+#define DBFILE_REMOVE(name)  remove((name))
>+#define DBFILE_UNLINK(name)  remove((name))
>+#else
>+#define DBFILE_REMOVE(name)  (PR_FAILURE == PR_Delete((name)) ? -1 : 0)
>+#define DBFILE_UNLINK(name)  (PR_FAILURE == PR_Delete((name)) ? -1 : 0)
>+#endif

(of course I stripped out some lines for this comment)

These two #define's are the same in their respective branches
(DBFILE_REMOVE(name)) == (DBFILE_UNLINK(name)) ...why, mistake?


>RCS file: /cvsroot/mozilla/dbm/include/mcom_db.h,v
>+#if defined(WINCE)
>+#include "nspr.h"
>+#endif
>===================================================================
>RCS file: /cvsroot/mozilla/dbm/tests/Makefile.in,v
>-LIBS		= $(EXTRA_DSO_LIBS)
>+LIBS		= $(EXTRA_DSO_LIBS) $(NSPR_LIBS) 

Should this be checked by platform rather than a unanimous check, due to nspr.h
being #if'ed?



>Index: src/hash.c
>===================================================================
>RCS file: /cvsroot/mozilla/dbm/src/hash.c,v
> #include <fcntl.h>
>+
> #include <stdio.h>

I should probably yell at you for the un-needed newline...


>+#if !defined(WINCE)
> 	if ((flags & O_ACCMODE) == O_WRONLY) {
> 		errno = EINVAL;
> 		RETURN_ERROR(ENOMEM, error0);
> 	}
>+#else
>+	if ((flags & PR_ACCMODE) == PR_WRONLY) {
>+		errno = EINVAL;
>+		RETURN_ERROR(ENOMEM, error0);
>+	}
>+#endif

why not:

#if !defined(WINCE)
	if ((flags & O_ACCMODE) == O_WRONLY) {
#else
	if ((flags & PR_ACCMODE) == PR_WRONLY) {
#endif
		errno = EINVAL;
		RETURN_ERROR(ENOMEM, error0);
	}

????

>@@ -599,22 +651,30 @@
...
> #else
>-	int    fd        = hashp->fp;
>-	off_t  file_size = lseek(fd, (off_t)0, SEEK_END);
>+	DBFILE_PTR    fd        = hashp->fp;
>+	off_t  file_size = DBFILE_SEEK(fd, (off_t)0, SEEK_END);
>+#if !defined(WINCE)
> 	HANDLE handle    = (HANDLE)_get_osfhandle(fd);
> 	BOOL   cool      = FlushFileBuffers(handle);
>+#else
>+    BOOL cool = TRUE;
>+    if(PR_FAILURE == PR_Sync(fd))
>+    {
>+        cool = FALSE;
>+    }
>+#endif

how about:

#else
   BOOL cool = (PR_FAILURE != PR_Sync(fd));
#endif


>@@ -773,10 +834,17 @@
> 		hashp->dbmerrno = errno = EINVAL;
> 		return (DBM_ERROR);
> 	}
>+#if !defined(WINCE)
> 	if ((hashp->flags & O_ACCMODE) == O_RDONLY) {
> 		hashp->dbmerrno = errno = EPERM;
> 		return (DBM_ERROR);
> 	}
>+#else
>+	if ((hashp->flags & PR_ACCMODE) == PR_RDONLY) {
>+		hashp->dbmerrno = errno = EPERM;
>+		return (DBM_ERROR);
>+	}
>+#endif

same #if-#else case idea with an if as before...


>@@ -809,10 +877,17 @@
>+#if !defined(WINCE)
> 	if ((hashp->flags & O_ACCMODE) == O_RDONLY) {
> 		hashp->dbmerrno = errno = EPERM;
> 		return (DBM_ERROR);
> 	}
>+#else
>+	if ((hashp->flags & PR_ACCMODE) == PR_RDONLY) {
>+		hashp->dbmerrno = errno = EPERM;
>+		return (DBM_ERROR);
>+	}
>+#endif

same again




>Index: src/hash_buf.c
>===================================================================
> #include <errno.h>
>+
> #include <stddef.h>

...newline



Hmm, while I was editing the comment I missed this:
hash.c Lines 168-192
....
>-#if defined(_WIN32) || defined(_WINDOWS) || defined (macintosh)  || defined(XP_OS2)
>+#if defined(_WIN32) || defined(_WINDOWS) || defined (macintosh)  || defined(XP_OS2_VACPP)

why the change to XP_OS2_VACPP are you sure its right (I'm not familiar)
=========================

These are just random comments in an area I am not too familiar with, dougt
just wanted me comments on the patch.

I only read the diff did not do any research on the value's of the functions he
changed for WINCE ;-)

Comment 3

[Doug Turner (:dougt)]

*** Bug 277869 has been marked as a duplicate of this bug. ***

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 170885 [details] [diff] [review]
Patch v.1

removing review on obsolete patch

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 170885 [details] [diff] [review]
Patch v.1

testing a much better (smaller) patch.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

F'it.  I just implemented:

_read 
_open
_write
_close
_unlink
_lseek

in the wince shunt library.  This compiles with against the newest shunt lib.

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.3

my port of the api's in comment 6 is somewhat implement.  I am working on
fixing this up. In the mean time, I think we should move forward with
Nelson's|Bythe's patch for DBM.

Comment 8

[Robert Relyea]

Comment on attachment 187973 [details] [diff] [review]
patch v.3

r+ rrelyea.

Possible future improvements:
The WINCE changes all changes to use NSPR. It might be good to make the define
USE_NSPR, but doing so would mean making sure it would work with other
platforms are defined as well (not just blinding changing WINCE to USE_NSPR).

There are several places where the ifdef simple changes how the flags are
processed. These could be illiminated by defining 'dbm generic flags' in the
header files and using those generic flags. (also not required for checkin).

Both changes would require additional review.

Comment 9

[Doug Turner (:dougt)]

there is a DBM_USING_NSPR_BRANCH.  maybe we should that.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch merge

Comment 11

[Doug Turner (:dougt)]

Attachment: patch merge v.5

that list patch was totally bogus

Comment 12

[Doug Turner (:dougt)]

It seams like a reasonable thing to remove the platform specific code in dbm and
use nspr directly.

Comment 13

[Doug Turner (:dougt)]

Attachment: includes relyea's comments

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188478 [details] [diff] [review]
patch merge v.5

renaming this patch so that  each patch has a unique description, and interdiff
can then diff two patches.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.7

same has patch v.6 but I added back the __remove_database function and call
sites.

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.8

same has patch v.6 but I added back the __remove_database function and call
sites.

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 188733 [details] [diff] [review]
patch v.7

this is a Makefile.in -- not a patch.

Comment 18

[Robert Relyea]

Comment on attachment 188734 [details] [diff] [review]
patch v.8

Provided you do the following:

1) There are some includes that are ifdef out WINCE. With the use of NSPR
several of these may no longer be necessary. We should elliminate them for all
platforms. (errno is an exception).

2) there are a few ifdefs like 'ifdef machintosh' and then a different check of
the O_ BITS. We should verify that this is still necessary with the PR_ flags
(I think they aren't).

3) verify that myLseek is necessary now that we are using PR_Seek().

bob

Comment 19

[Robert Relyea]

Oh, and do we need nsres anymore if NSS is the only user of dbm (NSS never uses
the nsres code.

bob

Comment 20

[Wan-Teh Chang]

Comment on attachment 188734 [details] [diff] [review]
patch v.8

NSS has a RDB interface for loading alternate database
plugins.

If DBM switches from POSIX I/O functions (open, read, write,
close) to NSPR I/O functions (PR_Open, PR_Read, PR_Write,
PR_Close), will this break the existing binary rdb.dll's?

Does RDB use any of the POSIX O_XXX flags?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188734 [details] [diff] [review]
patch v.8

Sorry it took me so long to review this.  I had to wait until I was
(nearly) done with all the freebl work for 3.11 first.	

I recognize some of this as coming from the DBM_USING_NSPR_BRANCH,
but still, I found several issues with this patch.  I think that 
several important changes made to DBM since DBM_USING_NSPR_BASE are 
un-done by this patch (new regressions).  

The diffs between revs 3.21 and 3.22 in hash.c fixed MANY problems 
with leaks, double-frees, and crashes due to using freed memory. 
They changed __hash_open and functions that it calls to correct
error paths.  The major changes were 
a) Remove the calls to hdestroy from alloc_segs(), because when
alloc_segs returns, the caller continues to rely on the contents
of the hashp struct.  
b) change __hash_open error paths to always goto "error1" at the 
bottom of the function, rather than returning in the middle, and 
c) change __hash_open to always call hdestroy() at label error1,
and nowhere else in __hash_open or the funtions it calls.

So, every place where this patch reintroduces a reference to the
label error0 or to hdestroy() should be regarded as a regression.

Finally, What is the state of errno for NSS in WinCE now?
Are we using the NSPR-based errno, which uses thread local storage?
Or are we using the errno hack that is not thread-safe?
I surely wish we were using the NSPR-based solution.

Comment 22

[Doug Turner (:dougt)]

bob -- 

I fixed up the wce / wince ifdef #include stuff.

I am not sure if machintosh is still needed -- is this defined for OSX or is a
more unixy flag used?  if it isn't used, I can remove all of it.

PR_Seek() doesn't zero fill on all platforms (iirc).


Nelson -- 

You are so right.  This path was simply a join and an resolution of some
conflicts.  I will post a new patch with the fixes that you pointed out.  I will
also clean up the usage of errno -- it seams to be used inconsisently in the
patch that I posted.

Also, the current errno implementation on WINCE is broken.  I will fix it so
that we use tls.  (basically ripping off what you did a few years ago.) 

New patch coming up...

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.9

Comment 24

[Doug Turner (:dougt)]

Attachment: patch v.10

After talking to relyea and josh ash (the mac guy), i removed the macintosh
defines.  these defines were only for <= os9.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192718 [details] [diff] [review]
patch v.9

canceling review request for obsolete patch

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192738 [details] [diff] [review]
patch v.10

Doug, was it your intent to remove all (or most) of the WinCE ifdefs from this
sour ce?  I ask because that appears to have happened in this latest patch, and
I thought you were doing this for WinCE.

Comment 27

[Doug Turner (:dougt)]

Yes.  I removed the WINCE specific stuff since the wince shunt layer provides
most of what was needed.  I also removed the macintosh defines as they are no
longer used in any build.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192738 [details] [diff] [review]
patch v.10

This is not a complete review of this patch.  I suggest you wait 
until I complete the review before attaching another patch.
I found one seeming glitch.

>@@ -55,131 +48,137 @@ static char sccsid[] = "@(#)hash_page.c	
>  *
>  * ROUTINES:
>  *
>  * External
>  *	__get_page
>  *	__add_ovflpage
>  * Internal
>  *	overflow_page
>  *	open_temp
>  */
>-#ifndef macintosh
> #include <sys/types.h>
>-#endif
>-
>-#if defined(macintosh)
> #include <unistd.h>
>-#endif

Previously, this file included <unistd.h> only on macintosh.
Now it does it on all platforms?  Including WinCE ?

Comment 29

[Doug Turner (:dougt)]

good catch, although I think it is safe, i will still remove it.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192738 [details] [diff] [review]
patch v.10

In this review, I focused mostly on this one file.

>Index: src/hash.c
>===================================================================
>RCS file: /cvsroot/mozilla/dbm/src/hash.c,v


> #include "watcomfx.h"

suggestion: I think we can drop the old watcom compiler compatibility
header file now. :)

>-#if !defined(_WIN32) && !defined(_WINDOWS) && !defined(macintosh) && !defined(XP_OS2_VACPP)
>+#if !defined(_WIN32) && !defined(_WINDOWS) && !defined(XP_OS2_VACPP)
> #include <sys/param.h>
> #endif
> 
>-#if !defined(macintosh)
> #ifdef XP_OS2_EMX
> #include <sys/types.h>
> #endif
>-#include <sys/stat.h>
>-#endif
> 
>-#if defined(macintosh)
>-#include <unix.h>
>-#include <unistd.h>
>-#endif
>+#include <sys/stat.h>
> 
>-#include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>

This comment/issue applies to numerous of the files patched in this patch.
Does WinCE's SDK now have these files? 
  <assert.h> 
  <fcntl.h> 
  <sys/stat.h> 
It didn't last time I worked with it. 
Yet those files are now unconditionally included.

Also, do we need fcntl.h or sys/stat.h at all any more, on any platform, 
given that we're now using NSPR for those functions?

>+#ifdef XP_OS2_VACPP
>+#include "types.h"
>+#define EPERM SOCEPERM
>+#endif
>+
> #include <assert.h>

IIRC, OS/2's "types.h" file is its equivalent/replacement for <sys/types.h>
We've dropped <sys/types.h> above.  So, I wonder if we can drop types.h too.

> 	new_table = 0;
>-	if (!file || (flags & O_TRUNC) 	|| (stat(file, &statbuf)  && (errno == ENOENT))) 
>-	{
>-		if (errno == ENOENT)
>-			errno = 0; /* Just in case someone looks at errno */
>-		new_table = 1;
>-	}
>-	else if(statbuf.st_mtime && statbuf.st_size == 0)
>+	if (!file || (flags & PR_TRUNCATE) ||
>+       (PR_GetFileInfo(file, &fileInfo)  &&
>+       (PR_GetError() == PR_FILE_NOT_FOUND_ERROR)) ||
>+       (fileInfo.modifyTime && fileInfo.size == 0))

Indentation problem.  There were LOTS of indentation problems in this patch.
The original source file used tab characters for indentation.  I suspect
that you edited it with an editor that set tab stops to 2 characters, and
that inserted spaces, not tabs, when editing.  Consequently, all such text
doesn't appear indented properly when viewed with most editors or email
programs or bugzilla diff or interdiff or ...


> 	if (new_table) {
>-		if (!init_hash(hashp, file, (HASHINFO *)info))
>-			RETURN_ERROR(errno, error1);
>+		if (!init_hash(hashp, file, (HASHINFO *)info)) {
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);
>+		}
> 	} else {
> 		/* Table already exists */
> 		if (info && info->hash)
> 			hashp->hash = info->hash;
> 		else
> 			hashp->hash = __default_hash;
> 
>-		hdrsize = read(hashp->fp, (char *)&hashp->hdr, sizeof(HASHHDR));
>-		if (hdrsize == -1)
>-			RETURN_ERROR(errno, error1);
>-		if (hdrsize != sizeof(HASHHDR))
>-			RETURN_ERROR(EFTYPE, error1);
>+		hdrsize = PR_Read(hashp->fp, (char *)&hashp->hdr, sizeof(HASHHDR));
> #if BYTE_ORDER == LITTLE_ENDIAN
> 		swap_header(hashp);
> #endif
>+		if (hdrsize < 0)
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);
>+
>+		if (hdrsize != sizeof(HASHHDR))
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);

Several comments here:
a) I think we do not want to call swap_header until we know that we've 
read in a full header.	
b) seems like the hdrsize < 0 test is redundant, and the "!= sizeof" test
is necessary and sufficient.  

> 		/* Verify file type, versions and hash function */
> 		if (hashp->MAGIC != HASHMAGIC)
>-			RETURN_ERROR(EFTYPE, error1);
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);
> #define	OLDHASHVERSION	1
> 		if (hashp->VERSION != HASHVERSION &&
> 		    hashp->VERSION != OLDHASHVERSION)
>-			RETURN_ERROR(EFTYPE, error1);
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);
> 		if (hashp->hash(CHARKEY, sizeof(CHARKEY)) != hashp->H_CHARKEY)
>-			RETURN_ERROR(EFTYPE, error1);
>-		if (hashp->NKEYS < 0) /* Old bad database. */
>-			RETURN_ERROR(EFTYPE, error1);
>+			RETURN_ERROR(PR_INVALID_ARGUMENT_ERROR, error1);


c) The hashp members spelled in all caps are all actually members of
hashp->hdr.
E.g.  hashp->NKEYS is actually hashp->hdr.nkeys .
If the hdr is invalid, we should ensure that hashp->save_file is zero before 
calling hdestroy at error1.  Otherwise, flush_meta (called from hdestroy) will 
do bad things. (We don't want to make matters worse by rewriting the bad header
to the file.  We can probably just set hashp->save_file to 0 at label error1
just before calling hdestroy.

d) Now that dbm is exclusively part of NSS, I believe we can use SEC_ERROR_XXX 
errors in addition to PR_xxxx_ERROR errors.  I believe that in this file, 
any time we encounter evidence that the content of the DB file was bad,
we should set the PR error code to SEC_ERROR_BAD_DATABASE rather than 
PR_INVALID_ARGUMENT_ERROR.


> error1:
>-	hdestroy(hashp);
>-	errno = save_errno;
>+    hdestroy(hashp);
> 	return (NULL);

indentation.


>-	return (hashp->fp);
>+	/* If DBM users really want the fd, they're going to have to change
>+	** to using a PRFileDesc *, or we're going to have to dig it out of
>+	** the PRFileDesc.  But for now ...
>+	*/
>+	PR_SetError(PR_NOT_IMPLEMENTED_ERROR, ENOSYS);
>+    return -1;
> }

indentation

> 	if(rv == DATABASE_CORRUPTED_ERROR)
>-	  {
>+    {
> #if defined(unix) && defined(DEBUG)
>-		printf("\n\nDBM Database has been corrupted, tell Lou...\n\n");
>+      printf("\n\nDBM Database has been corrupted, tell Lou...\n\n");

Let's not tell Lou, but say we did.  :)

> #endif
>-	  	__remove_database((DB *)dbp);
>-	  }
>-
>+      __remove_database((DB *)dbp);
>+    }
> 	return(rv);

indentation. 

> 	if(rv == DATABASE_CORRUPTED_ERROR)
>-	  {
>+    {
> #if defined(unix) && defined(DEBUG)
>-		printf("\n\nDBM Database has been corrupted, tell Lou...\n\n");
>+      printf("\n\nDBM Database has been corrupted, tell Lou...\n\n");
> #endif
>-	  	__remove_database((DB *)dbp);
>-	  }
>-
>-	return(rv);
>+      __remove_database((DB *)dbp);
>+    }
>+    return(rv);
> }

indentation.  I won't cite any of the other examples.

I think we should consider eliminating __remove_database. 
Removing a customer's DB might be what lawyers call an "actionable offense".

>Index: src/ndbm.c
>===================================================================
>RCS file: /cvsroot/mozilla/dbm/src/ndbm.c,v

IIRC, ndbm is an alternate API for dbm, and NSS doesn't use it at all.
I think (not sure) that all the dbm/src/n* files fall are unsed by NSS.
I suggest that we cvs remove any effectively dead files, so that we 
won't spend more time needlessly maintaining them.  


>Index: tests/lots.c
>===================================================================
>RCS file: /cvsroot/mozilla/dbm/tests/lots.c,v
>retrieving revision 3.13

>-	database = dbopen("test.db", O_RDWR | O_CREAT, 0644, DB_HASH, &hash_info);
>+	database = dbopen("test.db", PR_RDWR | PR_CREAT, 0644, DB_HASH, &hash_info);

This reminds me that there needs to be another patch to accompany 
this one, one that changes all NSS's users of dbopen to use the 
PR_ flags.  There are 11 calls to dbopen in NSS.  See
http://lxr.mozilla.org/security/search?string=+dbopen

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188471 [details] [diff] [review]
patch merge

canceling review request for obsolete patch

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188478 [details] [diff] [review]
patch merge v.5

canceling review request for obsolete patch

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 188714 [details] [diff] [review]
includes relyea's comments

canceling review request for obsolete patch

Comment 34

[Doug Turner (:dougt)]

I have incorporated all of your suggestion (except the tab vs. spaces -- which I
will soon).  

I think going from the nspr errors to the nss errors will make DBM dependent on
NSS.  It seams like a cycle that we don't want to create, right? 

Do you want me to post another patch, or should I wait for additional comments?
 Thanks again for reviewing this patch!

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

We think of dbm as being part of NSS now.  dbm is presently built as part of
NSS.  The makefiles that build it are now in mozilla/security/dbm, and they in 
turn are built by a target in NSS's top makefile mozilla/security/nss/Makefile.
We are contemplating copying DBM from mozilla/dbm to mozilla/security/dbm so
that it will become part of the "security" module, and cvs removing it from
mozilla/dbm so that it will no longer appear in other modules.  

Thinking out loud here, I think we want to treat dbm like any other subordinate
component of NSS.  NSS subdirectories already depend on each other's header
files.  NSS's makefiles build it in several passes, first building NSPR, then
DBM, and finally building NSS itself in two passes, one "exporting" header files
from NSS's component subdirectories, and then compiling those directories.  

I could see dbm being built as part of those same two passes that are now used
for the rest of NSS, so that when DBM is compiled, all the DBM and NSS "public"
headers have already been made available for inclusion in the other directories.

Since DBM is only called by NSS, I think it reasonable for it to use NSS error
code.  I see no problem in dbm using error code numbers from NSS header files.  
(I wouldn't want it to depend on NSS code, but using error numbers is another
matter).  In effect, NSS's error number space is already a reserved portion
of NSPR's error number space, and DBM returns error numbers from that space.

(Yes, I'm all done reviewing patch v.10)

Comment 36

[Doug Turner (:dougt)]

maybe we should make a seperate build to make hash.c use nss error codes as well
as another bug to move dbm somewhere else in the cvs tree?

Comment 37

[Julien Pierre]

Given that we are planning on replacing DBM with a new DB in NSS 3.12, is it
really worth doing all the work of going to new error codes just for the NSS
3.11 release ?
In 3.12, DBM will still be there, but only for migration purposes.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Today, DBM doesn't use NSPR and doesn't return NSPR error codes. 
It returns EFTYPE, a reasonable error code from libc's set of error codes,
for bad files.  It doesn't return EINVAL for bad file contents.

The patch changes dbm to use NSPR, and proposes to return NSPR's equivalent 
of EINVAL for bad file contents.  I protest that.  

I don't understand why we would ever want a separate build of dbm that used
non-NSS error codes, since NSS is the ONLY user of dbm!  

Doug are you planning to introduce another use of dbm (besides in NSS) for 
WinCE?

Comment 39

[Doug Turner (:dougt)]

i have no plans for dbm.  I think sql-lite is the fasionable choice for a db now
a-days.

Comment 40

[Doug Turner (:dougt)]

Attachment: patch v.11

this patch includes nelson's comments.	I also attempted to clean up the space
vs. tabs here.

Comment 41

[Doug Turner (:dougt)]

Attachment: mozilla/Makefile.in change

This patch basically moves DBM where it should be -- right next to where
security is built.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193187 [details] [diff] [review]
patch v.11

This patch still has whitespace issues.  Numerous new lines don't have enough
indentation to match the code around them.  But if that is fixed, I'm OK with
this patch.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193187 [details] [diff] [review]
patch v.11

I can't both review and superreview this bug.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Doug, are you going to try to get this bug fixed for NSS 3.11?
I think you're close to done with the coding now, yes?

Comment 45

[Doug Turner (:dougt)]

Comment on attachment 193187 [details] [diff] [review]
patch v.11

yeah, this is done.  Bob -- give me an SR, and I can land it.  

The only thing that I found that was broken with this patch was that I had
DBM_USING_NSPR defined in 
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/softoken/config.mk#97
unconditionally while testing.	When we land, we need to ensure that this is
always set or we will not be able to open any db's

Comment 46

[Robert Relyea]

Comment on attachment 193187 [details] [diff] [review]
patch v.11

sr+ relyea

I can always see new areas to attack, but the patch is complete and should land
as is.

One area for a future quick patch is to get rid of the long endian checks in
mcom_db.h since we are now including NSPR. NSPR already does to work to
determine byte order.

Comment 47

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 48

[Julien Pierre]

I tried patch v.11 to see if it would help me with the win64 problems as I was seeing some issues in the dbm area. The patch wouldn't compile on win64 without adding
#include <errno.h>
to many source files.

I also thought including nss.h and secerr.h from within dbm was wrong, and it was only needed for the definition of the SEC_ERROR_BAD_DATABASE constant, so I just removed those includes and copied the definition for that error where needed.

Once I got everything to build unfortunately, things didn't run right - I couldn't even create a DB with certutil, which was much worse than when building dbm from the trunk. If we ever want to revive this patch, more work will be needed on it.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Doug, do you have any further interested in this bug?
If not, please resolve it WONTFIX

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Has the patch for this bug been awaiting checkin since 2005?

Comment 51

[Doug Turner (:dougt)]

no interest.  i think we can use sqlite format exclusively.

Comment 52

[Wan-Teh Chang]

The new mozce shunt library should also make it unnecessary to fix this bug.