19.82 KB, patch
Nelson Bolyard (seldom reads bugmail): review-
|Details | Diff | Splinter Review|
11.67 KB, patch
Robert Relyea: review+
|Details | Diff | Splinter Review|
3.10 KB, patch
|Details | Diff | Splinter Review|
As I understand it, we can use sqlite instead which doesn't use the posix io functions that break dbm's build.
Created attachment 347361 [details] [diff] [review] patch v2
Comment on attachment 347361 [details] [diff] [review] patch v2 This patch makes changes to NSS that are desired by the Fennec project, but it makes them for all users who would build NSS on WinCE. In effect, Fennec assumes it is and will be the only use of NSS on WinCE. I think that if the Fennec project decides to omit parts of NSS from the build, the Makefiles should be ifdef FENNEC rather than (or in addition to) ifdef WINCE.
until someone fixes dbm for wince, no project will be able to use it.
> until someone fixes dbm for wince, That work was previously done, long ago. I'll bet it's still on a branch somewhere.
nelson, lets just add a configure flag that allows anyone do enable or disable DBM.
Created attachment 349382 [details] [diff] [review] uses --disable-dbm
thanks brad. this is great.
Comment on attachment 349382 [details] [diff] [review] uses --disable-dbm +[ --disable-dbm Disable building dbm], Should we be more specific and call this --disable-nss-dbm ? I hate having hard-to-decipher build flags. +ifndef NSS_DISABLE_DBM ifdef MOZ_MORK DIRS = mdb mork endif +endif Are NSS_DISABLE_DBM and MOZ_MORK mutually exclusive? --- a/dbm/Makefile.in I can't for the life of me figure out how the build gets into dbm/. Does NSS call back down into there? Seems silly to comment out the DIRS in this dir, we should just ask NSS not to build in this dir, if possible. r=me on the mozilla build system bits.
I object to NSS bugs being double-secret Confidential Mozilla Project Bugs. Any change made to NSS needs to have the scrutiny of all NSS developers. I lack the power to remove that flag, or I would. This proposed change simply stops building one of NSS's shared libs. It doesn't also stop building the code that attempts to use that lib. It doesn't even change NSS's default so that it defaults to using sql instead of dbm. So, the default behavior of NSS on WinCE with this patch will be to fail, due to inability to load a required shared lib, unless the app does one of the 3 methods by which to enable the use of sql. I don't think it is acceptable to make NSS fail by default on any platform. NSS builds dbm in the directory security/dbm. The make in that directory is invoked from the Makefile in security/nss. Look at the build_dbm target in security/nss/Makefile
(In reply to comment #11) > I object to NSS bugs being double-secret Confidential Mozilla Project Bugs. > Any change made to NSS needs to have the scrutiny of all NSS developers. > I lack the power to remove that flag, or I would. agreed. Doug, did you set it for a reason? > > This proposed change simply stops building one of NSS's shared libs. > It doesn't also stop building the code that attempts to use that lib. > It doesn't even change NSS's default so that it defaults to using sql > instead of dbm. So, the default behavior of NSS on WinCE with this patch > will be to fail, due to inability to load a required shared lib, unless the > app does one of the 3 methods by which to enable the use of sql. > I don't think it is acceptable to make NSS fail by default on any platform. We're not setting the default behavior of windows ce with this patch, instead creating a configure flag to disable dbm building since dbm uses posix io functions not available on windows ce. The app we're building (xulrunner and anything else built with the mozilla build system) sets the NSS_DEFAULT_DB_TYPE env var to sql. Is one of the other 2 ways a build flag that we can set so the configure flag can handle it?
it should NOT be confidential.
Thanks for dropping that flag. I don't object (at all!) to creating a build-time option to build NSS such that it always uses sql and never uses dbm. In fact, I welcome it. Such a project needs to: a) be configurable on all platforms, enabled through a "feature test macro" such as "NSS_SQL_ONLY" or "NSS_NO_DBM" that is not tied to any one platform (not WINCE). and b) change the behavior of NSS when built that way so that by default, even without a "sql:" prefix on the DB directory name and without NSS_DEFAULT_DB_TYPE in the environment, NSS will use sql, and not attempt to use dbm. I perceive the above two characteristics to be requirements for this RFE. In addition, there's a LOT more code than just the DBM directory that can be left out of such a configuration. The memory "footprint" of NSS could be reduced significantly when built that way. But I wouldn't require that for this RFE.
Changing the default form of DB used by NSS in the absence of a directory name prefix or NSS_DEFAULT_DB_TYPE envariable is a very simple fix. I think it's just one line. This one: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/sftkpars.c&rev=1.6&mark=549#545
NSS_DISABLE_DBM looks like it will do nicely for the FTM.
This patch conditionally removes mcom_db.h from a number of files in the certdb directory. If everything compliles correctly, I think we should always remove mcom_db.h from those files. I believe the are an ancient historical dreg. libnssdbm was put in it's own shared library precisely so it could be easy to drop from a distribution (as long as that distribution never needed to reference old NSS dbm databases). It also reduces the in memory footprint of applications that are not actively using the database. I agree such a distribution should make sql: the default. Nelson has correctly identified the correct line of code to change.
Comment on attachment 349382 [details] [diff] [review] uses --disable-dbm This is close, but a few changes are needed. There are certain files modified by this patch that I have excluded from my review because they are outside of NSS. Some reviewer for the module(s) that contain those files should review them. They are: mozilla/config/autoconf.mk.in mozilla/configure.in mozilla/db/Makefile.in mozilla/security/manager/Makefile.in I believe no change is required to mozilla/dbm/Makefile.in because AFAIK, that is a dead file. The makefiles actually used to build dbm are not in this directory but rather are in mozilla/security/dbm. I believe the lines you want to undo are: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/dbm/manifest.mn&rev=1.4&mark=47-48#38 and the way to do that is to add some lines in section 6 of mozilla/security/dbm/Makefile to conditionally redefine DIRS = $(NULL) http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/dbm/Makefile&rev=1.2&mark=73#63 because manifest.mn should have no ifdefs. Bob's right that, IFF mcom_db.h is not needed, those 8 #includes should just be deleted. But If they are needed, then rather than ifdef'ing the #include "mcom_db.h" in 8 files, I might suggest putting that ifdef into the mcom_db.h source file, bracketing the file's entire contents. To make that work, you would change the patch to the build_dbm target in mozilla/security/nss/Makefile to just make the export target, not the libs target. If mcom_db.h is not needed any more at all, then, rather than changing the build_dbm target in mozilla/security/nss/Makefile to do nothing (as this patch does), I would suggest changing it to output (echo) a string such as "skipping the build of DBM". Rather than modifying the contents of file mozilla/security/nss/lib/softoken/legacydb/Makefile to disable the build of that directory, you can just add about 3 lines into section 6 of file mozilla/security/nss/lib/softoken/Makefile that conditionally redefine DIRS = $(NULL) if the FTM is defined. Note: manifest.mn should not have ifdefs. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/Makefile&rev=1.6&mark=73#63
Created attachment 349795 [details] [diff] [review] removes mcom, removes dead makefile, fixes ruleset.mk, clears DIRS as nelson suggested we can carry the r=ted for the non-nss bits of this patch since they didn't change
Unsolicited comments: I'm not sure I would removed SDB_LEGACY altogether. It may be preferable to have applications fail when explicitly requesting opening dbm databases rather than silently moving to sql databases. Your changes a line 548 is sufficient to switch any applications that don't explicitly specify which database to open sql databases and your changes at line 593 are sufficient to prevent selection of dbm databases based on the user setting an environment variable. All other cases someone explicitly decided that the specific database to open was an old dbm database, so I think that case should simply fail (which it will when if fails to load libnssdbm3.so... er.. or nssdbm3.dll). If you do remove SDB_LEGACY, then you also need to remove SDB_MULTIACCESS. The now defunct multiaccess database code is implemented in nssdbm3 (with appropriate shared library decoration). Multiaccess loaded an external module that had a DBM programming interface, so it used the DBM mapping code. bob
Comment on attachment 349795 [details] [diff] [review] removes mcom, removes dead makefile, fixes ruleset.mk, clears DIRS as nelson suggested This patch is really close. I'm delighted to learn that no sources outside of nss/lib/softoken/legacydb and dbm itself need mcom_db.h. Truly. I agree with Bob 100% that an attempt to use the dbm: or multiaccess: prefixes (which explicitly request the use of something like the old DB) should return an error instead of silently using cert9 and sql instead. The change to the default implementation should suffice. So, do NOT ifdef out SDB_LEGACY. Thanks. The proposed change to security/coreconf/ruleset.mk simply breaks the build. It causes the "for" statement to evaluate $directory as a single value which is a string that is the concatenation of all the DIRS, e.g. as "lib cmd", and of course, you cannot cd to "lib cmd". So, that change is bad. r- for that.
Created attachment 349937 [details] [diff] [review] leaves SDB_LEGACY alone note that ruleset.mk doesn't handled an empty DIRS out of the box. If it encounters and empty DIRS, sh can't parse the script (at least on windows) because it evaluates as "for directory in ; do" and ';' is not a valid list. I've appended "dummy" to the list of dirs to avoid this, which will get ignored after the following test and should be harmless. If there's a more elegant way to do this, please let me know.
Created attachment 350095 [details] [diff] [review] patch - also builds nss/cmd (checked in on trunk) I was ready to give r+ to the preceding patch, but I applied it to the trunk and tried to build NSS, and found that the cmd directory did not build. So, I fixed that and created a new patch. Bob, please review. This patch is basically the same as the previous one, except a) It only modifies files that are part of NSS, b) Instead of adding "dummy" to the for command in coreconf/ruleset.mk it changes the makefiles that are defining DIR = $(NULL) to instead define DIR = dummy . It does not change ruleset.mk . c) it also modifies security/nss/cmd/platlibs.mk so that the utilities all build when NSS_DISABLE_DBM is defined. I have successfully built the CVS trunk of NSS, including libs and cmds, both with and without NSS_DISABLE_DBM, with this patch. I'd like the Fennec guys to test this patch, and confirm here that it works for them. Remember, this patch includes only the NSS files.
Comment on attachment 349937 [details] [diff] [review] leaves SDB_LEGACY alone I almost gave r+ to this patch, but then I tried it. I pulled the trunk and applied it, and tried building both with and without NSS_DISABLE_DBM. I found that with that symbol defined, the NSS utility programs don't build. I have produced a new patch that fixes it. It awaits review.
Created attachment 350111 [details] [diff] [review] non-nss parts of previousl patch [Checked in: Comment 29 & 31]
Nelson, you patch built just fine on windows ce. I've attached the non-nss parts of the previous patch and carried ted's review.
Comment on attachment 350095 [details] [diff] [review] patch - also builds nss/cmd (checked in on trunk) r+ rrelyea
Comment on attachment 350095 [details] [diff] [review] patch - also builds nss/cmd (checked in on trunk) Checked in on trunk. dbm/Makefile.in; new: 1.10; previous: 1.9 security/coreconf/config.mk; new: 1.26; previous: 1.25 security/dbm/Makefile; new: 1.3; previous: 1.2 security/nss/Makefile; new: 1.36; previous: 1.35 security/nss/cmd/platlibs.mk; new: 1.59; previous: 1.58 security/nss/lib/certdb/certdb.c; new: 1.95; previous: 1.94 security/nss/lib/certdb/genname.c; new: 1.37; previous: 1.36 security/nss/lib/certdb/stanpcertdb.c; new: 1.83; previous: 1.82 security/nss/lib/certdb/xauthkid.c; new: 1.8; previous: 1.7 security/nss/lib/certdb/xbsconst.c; new: 1.6; previous: 1.5 security/nss/lib/certdb/xconst.c; new: 1.13; previous: 1.12 security/nss/lib/pk11wrap/secmodi.h; new: 1.31; previous: 1.30 security/nss/lib/softoken/Makefile; new: 1.7; previous: 1.6 security/nss/lib/softoken/pkcs11.c; new: 1.155; previous: 1.154 security/nss/lib/softoken/sftkpars.c; new: 1.7; previous: 1.6 security/nss/lib/softoken/legacydb/config.mk; new: 1.8; previous: 1.7
non-nss patch pushed to m-c http://hg.mozilla.org/mozilla-central/rev/20a011760de7
Comment on attachment 350111 [details] [diff] [review] non-nss parts of previousl patch [Checked in: Comment 29 & 31] a191=beltzner
this landed on the 1.9.1 branch. adding keyword fixed1.9.1
Created attachment 546457 [details] [diff] [review] Supplemental cleanup patch Brad: This patch cleans up your NSS patch (attachment 350095 [details] [diff] [review]) that Nelson checked in for you. 1. In mozilla/security/nss/Makefile, add '@' to the echo command so that gmake won't echo the echo command. I also change ifndef to ifdef and reverse the two branches. 2. mozilla/security/nss/lib/softoken/legacydb/config.mk does not need to check NSS_DISABLE_DBM because that entire directory is skipped for NSS_DISABLE_DBM.
Comment on attachment 546457 [details] [diff] [review] Supplemental cleanup patch Review of attachment 546457 [details] [diff] [review]: -----------------------------------------------------------------
Comment on attachment 546457 [details] [diff] [review] Supplemental cleanup patch I moved this patch to bug 700066.