Re: Anyone want to help make zsh/db/gdbm work?

[Peter Stephenson <p.stephenson@samsung.com>]

On Mon, 2 Feb 2015 09:46:43 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Fri, 30 Jan 2015 12:03:04 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > If you're actually going to look up the parameter first, you should also
> > delay the gdbm_open() until you've verified the parameter.
> 
> There's another point here that if unsetting the old parameter happens
> to untie an existing database, possibly even the same one, it needs to
> be done before the new open.

This moves things around.  I kept the code so I could test the return
value, but as noted in the comments it would currently be good enough to
use unsetparam() and test errflag.

I can't see why pmname was duplicated; createparam() doesn't require
this and doesn't ever free what you passed in.  It was suspicious that
the duplicated value of pmname was used in an error message just before
a return: either the value was invalid, or it needed to be freed, and I
think the latter.  I've simply removed the duplication.

diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 2e2bd3a..76d4751 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -85,14 +85,7 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
     }
 
     resource_name = OPT_ARG(ops, 'f');
-
-    dbf = gdbm_open(resource_name, 0, read_write, 0666, 0);
-    if(!dbf) {
-	zwarnnam(nam, "error opening database file %s", resource_name);
-	return 1;
-    }
-
-    pmname = ztrdup(*args);
+    pmname = *args;
 
     if ((tied_param = (Param)paramtab->getnode(paramtab, pmname)) &&
 	!(tied_param->node.flags & PM_UNSET)) {
@@ -100,13 +93,24 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 	 * Unset any existing parameter.  Note there's no implicit
 	 * "local" here, but if the existing parameter is local
 	 * that will be reflected in the new one.
+	 *
+	 * We need to do this before attempting to open the DB
+	 * in case this variable is already tied to a DB.
+	 *
+	 * This can fail if the variable is readonly or restricted.
+	 * We could call unsetparam() and check errflag instead
+	 * of the return status.
 	 */
-	if (unsetparam_pm(tied_param, 0, 1)) {
-	    zsfree(pmname);
-	    gdbm_close(dbf);
+	if (unsetparam_pm(tied_param, 0, 1))
 	    return 1;
-	}
     }
+
+    dbf = gdbm_open(resource_name, 0, read_write, 0666, 0);
+    if(!dbf) {
+	zwarnnam(nam, "error opening database file %s", resource_name);
+	return 1;
+    }
+
     if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys,
 					 pmflags))) {
         zwarnnam(nam, "cannot create the requested parameter %s", pmname);