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

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

[Bart Schaefer]

This module has been sitting around forever, completely undocumented.  The
basic functions work; I tweaked a couple of things and wrote a short page
of documentation, which explains about this:

schaefer<507> typeset GDBMDB
schaefer<508> ztie -d db/gdbm -f gdbmdb GDBMDB
ztie: cannot create the requested parameter GDBMDB

However, there's still this:

schaefer<518> typeset GDBMDB
schaefer<519> (){ local GDBMDB; unset GDBMDB; ztie -d db/gdbm -f gdbmdb GDBMDB }
 ../../zsh-5.0/Src/params.c:4914: BUG: in restoring scope of special parameter
zsh: segmentation fault (core dumped)  Src/zsh


This happens because ztie/zuntie directly manipulate the global paramtab.
Somebody else may be able to patch that up quicker than I.


diff --git a/Doc/Makefile.in b/Doc/Makefile.in
index 41af4a3..a420781 100644
--- a/Doc/Makefile.in
+++ b/Doc/Makefile.in
@@ -60,7 +60,7 @@ MODDOCSRC = \
 Zsh/mod_attr.yo Zsh/mod_cap.yo Zsh/mod_clone.yo \
 Zsh/mod_compctl.yo Zsh/mod_complete.yo Zsh/mod_complist.yo \
 Zsh/mod_computil.yo Zsh/mod_curses.yo \
-Zsh/mod_datetime.yo Zsh/mod_deltochar.yo \
+Zsh/mod_datetime.yo Zsh/mod_db_gdbm.yo Zsh/mod_deltochar.yo \
 Zsh/mod_example.yo Zsh/mod_files.yo Zsh/mod_langinfo.yo \
 Zsh/mod_mapfile.yo Zsh/mod_mathfunc.yo Zsh/mod_newuser.yo \
 Zsh/mod_parameter.yo Zsh/mod_pcre.yo Zsh/mod_regex.yo \
diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 9a2a7a5..f079094 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -39,9 +39,7 @@
 
 #include <gdbm.h>
 
-#if 0 /* what is this for? */
 static char *backtype = "db/gdbm";
-#endif
 
 static const struct gsu_scalar gdbm_gsu =
 { gdbmgetfn, gdbmsetfn, gdbmunsetfn };
@@ -60,33 +58,38 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
     Param tied_param;
 
     if(!OPT_ISSET(ops,'d')) {
-        zwarnnam(nam, "you must pass `-d db/gdbm' to ztie", NULL);
+        zwarnnam(nam, "you must pass `-d %s'", backtype);
 	return 1;
     }
     if(!OPT_ISSET(ops,'f')) {
-        zwarnnam(nam, "you must pass `-f' with a filename to ztie", NULL);
+        zwarnnam(nam, "you must pass `-f' with a filename", NULL);
 	return 1;
     }
 
     /* Here should be a lookup of the backend type against
      * a registry.
      */
+    if (strcmp(OPT_ARG(ops, 'd'), backtype) != 0) {
+        zwarnnam(nam, "unsupported backend type `%s'", OPT_ARG(ops, 'd'));
+	return 1;
+    }
 
     pmname = ztrdup(*args);
 
     resource_name = OPT_ARG(ops, 'f');
 
-    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
-        zwarnnam(nam, "cannot create the requested parameter name", NULL);
-	return 1;
-    }
-
     dbf = gdbm_open(resource_name, 0, GDBM_WRCREAT | GDBM_SYNC, 0666, 0);
     if(!dbf) {
         zwarnnam(nam, "error opening database file %s", resource_name);
 	return 1;
     }
 
+    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
+        zwarnnam(nam, "cannot create the requested parameter %s", pmname);
+	gdbm_close(dbf);
+	return 1;
+    }
+
     tied_param->u.hash->tmpdata = (void *)dbf;
 
     return 0;
@@ -98,19 +101,25 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
 {
     Param pm;
     GDBM_FILE dbf;
-
-    pm = (Param) paramtab->getnode(paramtab, args[0]);
-    if(!pm) {
-        zwarnnam(nam, "cannot untie %s", args[0]);
-	return 1;
+    char *pmname;
+    int ret = 0;
+
+    for (pmname = *args; *args++; pmname = *args) {
+	pm = (Param) paramtab->getnode(paramtab, pmname);
+	if(!pm) {
+	    zwarnnam(nam, "cannot untie %s", pmname);
+	    ret = 1;
+	    continue;
+	}
+
+	dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
+	gdbm_close(dbf);
+	/* free(pm->u.hash->tmpdata); */
+	pm->u.hash->tmpdata = NULL;
+	paramtab->removenode(paramtab, pm->node.nam);
     }
 
-    dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
-    gdbm_close(dbf);
-/*    free(pm->u.hash->tmpdata); */
-    paramtab->removenode(paramtab, pm->node.nam);
-
-    return 0;
+    return ret;
 }
 
 /**/

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

[ZyX]

23.01.2015, 08:20, "Bart Schaefer" <schaefer@brasslantern.com>:
> This module has been sitting around forever, completely undocumented.  The
> basic functions work; I tweaked a couple of things and wrote a short page
> of documentation, which explains about this:
>
> schaefer<507> typeset GDBMDB
> schaefer<508> ztie -d db/gdbm -f gdbmdb GDBMDB
> ztie: cannot create the requested parameter GDBMDB
>
> However, there's still this:
>
> schaefer<518> typeset GDBMDB
> schaefer<519> (){ local GDBMDB; unset GDBMDB; ztie -d db/gdbm -f gdbmdb GDBMDB }
>  ../../zsh-5.0/Src/params.c:4914: BUG: in restoring scope of special parameter
> zsh: segmentation fault (core dumped)  Src/zsh
>
> This happens because ztie/zuntie directly manipulate the global paramtab.
> Somebody else may be able to patch that up quicker than I.
>
> diff --git a/Doc/Makefile.in b/Doc/Makefile.in
> index 41af4a3..a420781 100644
> --- a/Doc/Makefile.in
> +++ b/Doc/Makefile.in
> @@ -60,7 +60,7 @@ MODDOCSRC = \
>  Zsh/mod_attr.yo Zsh/mod_cap.yo Zsh/mod_clone.yo \
>  Zsh/mod_compctl.yo Zsh/mod_complete.yo Zsh/mod_complist.yo \
>  Zsh/mod_computil.yo Zsh/mod_curses.yo \
> -Zsh/mod_datetime.yo Zsh/mod_deltochar.yo \
> +Zsh/mod_datetime.yo Zsh/mod_db_gdbm.yo Zsh/mod_deltochar.yo \

It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do not see it in the patch, only a small reference here.

>  Zsh/mod_example.yo Zsh/mod_files.yo Zsh/mod_langinfo.yo \
>  Zsh/mod_mapfile.yo Zsh/mod_mathfunc.yo Zsh/mod_newuser.yo \
>  Zsh/mod_parameter.yo Zsh/mod_pcre.yo Zsh/mod_regex.yo \
> diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
> index 9a2a7a5..f079094 100644
> --- a/Src/Modules/db_gdbm.c
> +++ b/Src/Modules/db_gdbm.c
> @@ -39,9 +39,7 @@
>
>  #include <gdbm.h>
>
> -#if 0 /* what is this for? */
>  static char *backtype = "db/gdbm";
> -#endif
>
>  static const struct gsu_scalar gdbm_gsu =
>  { gdbmgetfn, gdbmsetfn, gdbmunsetfn };
> @@ -60,33 +58,38 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
>      Param tied_param;
>
>      if(!OPT_ISSET(ops,'d')) {
> -        zwarnnam(nam, "you must pass `-d db/gdbm' to ztie", NULL);
> +        zwarnnam(nam, "you must pass `-d %s'", backtype);
>          return 1;
>      }
>      if(!OPT_ISSET(ops,'f')) {
> -        zwarnnam(nam, "you must pass `-f' with a filename to ztie", NULL);
> +        zwarnnam(nam, "you must pass `-f' with a filename", NULL);
>          return 1;
>      }
>
>      /* Here should be a lookup of the backend type against
>       * a registry.
>       */
> +    if (strcmp(OPT_ARG(ops, 'd'), backtype) != 0) {
> +        zwarnnam(nam, "unsupported backend type `%s'", OPT_ARG(ops, 'd'));
> + return 1;
> +    }
>
>      pmname = ztrdup(*args);
>
>      resource_name = OPT_ARG(ops, 'f');
>
> -    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
> -        zwarnnam(nam, "cannot create the requested parameter name", NULL);
> - return 1;
> -    }
> -
>      dbf = gdbm_open(resource_name, 0, GDBM_WRCREAT | GDBM_SYNC, 0666, 0);
>      if(!dbf) {
>          zwarnnam(nam, "error opening database file %s", resource_name);
>          return 1;
>      }
>
> +    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
> +        zwarnnam(nam, "cannot create the requested parameter %s", pmname);
> + gdbm_close(dbf);
> + return 1;
> +    }
> +
>      tied_param->u.hash->tmpdata = (void *)dbf;
>
>      return 0;
> @@ -98,19 +101,25 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
>  {
>      Param pm;
>      GDBM_FILE dbf;
> -
> -    pm = (Param) paramtab->getnode(paramtab, args[0]);
> -    if(!pm) {
> -        zwarnnam(nam, "cannot untie %s", args[0]);
> - return 1;
> +    char *pmname;
> +    int ret = 0;
> +
> +    for (pmname = *args; *args++; pmname = *args) {
> + pm = (Param) paramtab->getnode(paramtab, pmname);
> + if(!pm) {
> +    zwarnnam(nam, "cannot untie %s", pmname);
> +    ret = 1;
> +    continue;
> + }
> +
> + dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
> + gdbm_close(dbf);
> + /* free(pm->u.hash->tmpdata); */
> + pm->u.hash->tmpdata = NULL;
> + paramtab->removenode(paramtab, pm->node.nam);
>      }
>
> -    dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
> -    gdbm_close(dbf);
> -/*    free(pm->u.hash->tmpdata); */
> -    paramtab->removenode(paramtab, pm->node.nam);
> -
> -    return 0;
> +    return ret;
>  }
>
>  /**/

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

[Bart Schaefer]

On Jan 23,  8:37am, ZyX wrote:
}
} It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do
} not see it in the patch, only a small reference here.

Thanks.  (Though it would be nice if you actually trimmed out some of
the quoted context instead of just sticking a single sentence deep in the
middle of it.)

Actually what I didn't realize was that one has to diff against
origin/master for new files, rather than against the local master.


diff --git a/Doc/Zsh/mod_db_gdbm.yo b/Doc/Zsh/mod_db_gdbm.yo
new file mode 100644
index 0000000..6065f86
--- /dev/null
+++ b/Doc/Zsh/mod_db_gdbm.yo
@@ -0,0 +1,27 @@
+COMMENT(!MOD!zsh/db/gdbm
+Builtins for managing associative array parameters tied to GDBM databases.
+!MOD!)
+The tt(zsh/db/gdbm) module is used to create "tied" associative arrays
+that interface to database files.  If the GDBM interface is not available,
+the builtins defined by this module will report an error.  This module is
+also intended as a prototype for creating additional database interfaces,
+so the tt(ztie) builtin may move to a more generic module in the future.
+
+The builtins in this module are:
+
+startitem()
+findex(ztie)
+cindex(tied array, creating)
+item(tt(ztie -d db/gdbm -f) var(filename) var(arrayname))(
+Open the GDBM database identified by var(filename) and, if successful,
+create the associative array var(arrayname) linked to the file.  Note
+that var(arrayname) must be unset at the time tt(ztie) is called, and
+is always created as a global parameter (as if with `tt(typeset -g)').
+)
+findex(zuntie)
+cindex(tied array, destroying)
+item(tt(zuntie) var(arrayname) ...)(
+Close the GDBM database associated with each var(arrayname) and then
+unset the variable.
+)
+enditem()

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

[ZyX]

23.01.2015, 09:00, "Bart Schaefer" <schaefer@brasslantern.com>:
> On Jan 23,  8:37am, ZyX wrote:
> }
> } It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do
> } not see it in the patch, only a small reference here.
>
> Thanks.  (Though it would be nice if you actually trimmed out some of
> the quoted context instead of just sticking a single sentence deep in the
> middle of it.)
>
> Actually what I didn't realize was that one has to diff against
> origin/master for new files, rather than against the local master.

I have run into this problem a few times: by default git diffs against index (not the current revision) and when you do `git add` then file is added to the index as a whole making just `git diff` not present it. I believe this is why `git add` has `--intent-to-add` option that just adds path with an empty contents so that after `git add --intent-to-add path` `git diff` will show the full diff. I prefer to use my aurum Vim plugin though: it tries to work as if git had no index and always diffs against HEAD or some revision, mainly because it was originally developed for mercurial and I wanted uniform interface.

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

[Peter Stephenson]

On Fri, 23 Jan 2015 18:49:14 +0300
ZyX <kp-pav@yandex.ru> wrote:
> 23.01.2015, 09:00, "Bart Schaefer" <schaefer@brasslantern.com>:
> > On Jan 23,  8:37am, ZyX wrote:
> > } It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do
> > } not see it in the patch, only a small reference here.
> >
> > Actually what I didn't realize was that one has to diff against
> > origin/master for new files, rather than against the local master.
> 
> I have run into this problem a few times: by default git diffs against
> index (not the current revision) and when you do `git add` then file
> is added to the index as a whole making just `git diff` not present
> it.

The standard fix for this is "git diff --staged" (or --cached) but you
have to remember --- it's particular confusing if you've got some stuff
staged and some not as you only get to see diffs for one or other.  I'm
not sure it's quite what Bart is talking about as that seems to be
about committed files.

pws

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

[ZyX]

23.01.2015, 22:48, "Peter Stephenson" <p.w.stephenson@ntlworld.com>:
> On Fri, 23 Jan 2015 18:49:14 +0300
> ZyX <kp-pav@yandex.ru> wrote:
>>  23.01.2015, 09:00, "Bart Schaefer" <schaefer@brasslantern.com>:
>>>  On Jan 23,  8:37am, ZyX wrote:
>>>  } It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do
>>>  } not see it in the patch, only a small reference here.
>>>
>>>  Actually what I didn't realize was that one has to diff against
>>>  origin/master for new files, rather than against the local master.
>>  I have run into this problem a few times: by default git diffs against
>>  index (not the current revision) and when you do `git add` then file
>>  is added to the index as a whole making just `git diff` not present
>>  it.
>
> The standard fix for this is "git diff --staged" (or --cached) but you
> have to remember --- it's particular confusing if you've got some stuff
> staged and some not as you only get to see diffs for one or other.  I'm
> not sure it's quite what Bart is talking about as that seems to be
> about committed files.

No. Consider you do two things:

1. Modify file foo that is present in HEAD.
2. `git add` file bar which was not present in HEAD.

Possible results of `git diff` after these actions:

1. If you do `git diff` it will show you only modifications made to `foo`.
2. If you do `git diff --cached` you will only see bar contents in a diff. 
3. If you do `git diff HEAD` you will see both.

Whether you need to do one or the other depends on how you are going to commit and your workflow in general.

To diff committed files you need to use `git diff 'revspec^..revspec'` (yet another point I don’t like git for: even subversion is only asking you to specify refspec once. Do not tell me about aliases (shell and git), shell functions and other stuff, it is not fun to set them up everywhere I need git).

>
> pws

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

[Aaron Schrab]

At 23:00 +0300 23 Jan 2015, ZyX <kp-pav@yandex.ru> wrote:
>To diff committed files you need to use `git diff 'revspec^..revspec'` 

Or you can do `git show revspec`.  This will include the commit message 
and metadata as well as the diff, but I generally don't find that to be 
a problem; generally I consider that to be a bonus.  You could add 
`--pretty=format:` to get just the diff.

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

[Mikael Magnusson]

On Fri, Jan 23, 2015 at 8:47 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Fri, 23 Jan 2015 18:49:14 +0300
> ZyX <kp-pav@yandex.ru> wrote:
>> 23.01.2015, 09:00, "Bart Schaefer" <schaefer@brasslantern.com>:
>> > On Jan 23,  8:37am, ZyX wrote:
>> > } It looks like if you forgot to `git add` Doc/Zsh/mod_db_gdbm.yo. I do
>> > } not see it in the patch, only a small reference here.
>> >
>> > Actually what I didn't realize was that one has to diff against
>> > origin/master for new files, rather than against the local master.
>>
>> I have run into this problem a few times: by default git diffs against
>> index (not the current revision) and when you do `git add` then file
>> is added to the index as a whole making just `git diff` not present
>> it.
>
> The standard fix for this is "git diff --staged" (or --cached) but you
> have to remember --- it's particular confusing if you've got some stuff
> staged and some not as you only get to see diffs for one or other.  I'm
> not sure it's quite what Bart is talking about as that seems to be
> about committed files.

The best way to show a diff to someone is to finish what you're doing,
commit it, and then use git format-patch -1. If you really want to
diff the working tree without committing, git diff HEAD will always
show index+worktree diffed to the last commit (eg, what would be
committed with git commit -a), in addition to the other two variants
mentioned already (git diff: worktree to index, git diff --cached:
index to last commit).

-- 
Mikael Magnusson

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

[Peter Stephenson]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

On 23 January 2015 at 05:19, Bart Schaefer <schaefer@brasslantern.com>
wrote:
> However, there's still this:
>
> schaefer<518> typeset GDBMDB
> schaefer<519> (){ local GDBMDB; unset GDBMDB; ztie -d db/gdbm -f gdbmdb
GDBMDB }
>  ../../zsh-5.0/Src/params.c:4914: BUG: in restoring scope of special
parameter
> zsh: segmentation fault (core dumped)  Src/zsh

I think this is better, but you may still be able to find ways it
fails.  It's possibly a bit more paranoid than it needs to be;
the PM_REMOVABLE flag is probably the key.

However, in any case I think the way ztie and zuntie work play
fast and loose with parameters.  Try

typeset GDBMB-foo
(){
  local GDBMDB
  unset GDBMDB
  ztie -d db/gdbm -f gdbmdb GDBMDB
  zuntie GDBMDB
}
print $GDBMB

Note there's now a zuntie there.  This still doesn't appear to work.
It now does without the zuntie, though, at least for the most obvious
limited value of "work".

diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index f079094..d75af98 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -43,6 +43,9 @@ static char *backtype = "db/gdbm";

 static const struct gsu_scalar gdbm_gsu =
 { gdbmgetfn, gdbmsetfn, gdbmunsetfn };
+/**/
+static const struct gsu_hash gdbm_hash_gsu =
+{ hashgetfn, hashsetfn, gdbmhashunsetfn };

 static struct builtin bintab[] = {
     BUILTIN("ztie", 0, bin_ztie, 1, -1, 0, "d:f:", NULL),
@@ -84,12 +87,14 @@ bin_ztie(char *nam, char **args, Options ops,
UNUSED(int func))
     return 1;
     }

-    if (!(tied_param = createspecialhash(pmname, &getgdbmnode,
&scangdbmkeys, 0))) {
+    if (!(tied_param = createspecialhash(pmname, &getgdbmnode,
&scangdbmkeys,
+                     PM_REMOVABLE))) {
         zwarnnam(nam, "cannot create the requested parameter %s", pmname);
     gdbm_close(dbf);
     return 1;
     }

+    tied_param->gsu.h = &gdbm_hash_gsu;
     tied_param->u.hash->tmpdata = (void *)dbf;

     return 0;
@@ -225,6 +230,25 @@ scangdbmkeys(HashTable ht, ScanFunc func, int flags)

 }

+/**/
+static void
+gdbmhashunsetfn(Param pm, UNUSED(int exp))
+{
+    GDBM_FILE dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
+
+    if (!dbf) /* paranoia */
+    return;
+
+    gdbm_close(dbf);
+    pm->u.hash->tmpdata = NULL;
+
+    /* hash table is now normal, so proceed normally... */
+    pm->node.flags &= ~PM_SPECIAL;
+    pm->gsu.h = &stdhash_gsu;
+    pm->gsu.h->setfn(pm, NULL);
+    pm->node.flags |= PM_UNSET;
+}
+
 #else
 # error no gdbm
 #endif /* have gdbm */

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

[Peter Stephenson]

On Mon, 26 Jan 2015 12:11:16 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> typeset GDBMB-foo
> (){
>   local GDBMDB
>   unset GDBMDB
>   ztie -d db/gdbm -f gdbmdb GDBMDB
>   zuntie GDBMDB
> }
> print $GDBMB
> 
> Note there's now a zuntie there.  This still doesn't appear to work.
> It now does without the zuntie, though, at least for the most obvious
> limited value of "work".

That should have been an "=" in the top line.  I think that was webmail
again.  (Certainly not not my fault, I wouldn't make misteaks in emall.)

I tried this again and it was differently screwed up from what I
expected.  However, to cut a long story short, I think the following
fixes all the cases I know about.  It may fix a leak with zuntie:  I
don't think it was freeing the parameter it removed from the table.
Turning it into a standard unset should in principle make it more
foolproof.

I've also made zuntie safer: it checks the parameter was indeed tied
before.

The fix in module.c would affect special hashes loaded by the parameter
module.  However, in that case it was highly atypical for the special to 
hide an existing local variable so it's much less likely to have been
seen.

Could do with playing with, tests, etc., but I think I'll leave it at
this.

diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index d75af98..a6027de 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -105,7 +105,6 @@ static int
 bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
 {
     Param pm;
-    GDBM_FILE dbf;
     char *pmname;
     int ret = 0;
 
@@ -116,12 +115,18 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
 	    ret = 1;
 	    continue;
 	}
+	if (pm->gsu.h != &gdbm_hash_gsu) {
+	    zwarnnam(nam, "not a tied gdbm hash: %s", pmname);
+	    ret = 1;
+	    continue;
+	}
 
-	dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
-	gdbm_close(dbf);
-	/* free(pm->u.hash->tmpdata); */
-	pm->u.hash->tmpdata = NULL;
-	paramtab->removenode(paramtab, pm->node.nam);
+	queue_signals();
+	if (unsetparam_pm(pm, 0, 1)) {
+	    /* assume already reported */
+	    ret = 1;
+	}
+	unqueue_signals();
     }
 
     return ret;
diff --git a/Src/params.c b/Src/params.c
index 64c78bd..e8a9010 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -927,7 +927,15 @@ shempty(void)
 {
 }
 
-/* Create a simple special hash parameter. */
+/*
+ * Create a simple special hash parameter.
+ *
+ * This is for hashes added internally --- it's not possible to add
+ * special hashes from shell commands.  It's currently used
+ * - by addparamdef() for special parameters in the zsh/parameter
+ *   module
+ * - by ztie for special parameters tied to databases.
+ */
 
 /**/
 mod_export Param
@@ -939,7 +947,22 @@ createspecialhash(char *name, GetNodeFunc get, ScanTabFunc scan, int flags)
     if (!(pm = createparam(name, PM_SPECIAL|PM_HASHED|flags)))
 	return NULL;
 
-    pm->level = pm->old ? locallevel : 0;
+    /*
+     * If there's an old parameter, we'll put the new one at
+     * the current locallevel, so that the old parameter is
+     * exposed again after leaving the function.  Otherwise,
+     * we'll leave it alone.  Usually this means the parameter
+     * will stay in place until explicitly unloaded, however
+     * if the parameter was previously unset within a function
+     * we'll inherit the level of that function and follow the
+     * standard convention that the parameter remains local
+     * even if unset.
+     *
+     * These semantics are similar to those of a normal parameter set
+     * within a function without a local definition.
+     */
+    if (pm->old)
+	pm->level = locallevel;
     pm->gsu.h = (flags & PM_READONLY) ? &stdhash_gsu :
 	&nullsethash_gsu;
     pm->u.hash = ht = newhashtable(0, name, NULL);

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

[Bart Schaefer]

On Jan 29,  8:46pm, Peter Stephenson wrote:
} Subject: Re: Anyone want to help make zsh/db/gdbm work?
}
} I tried this again and it was differently screwed up from what I
} expected.  However, to cut a long story short, I think the following
} fixes all the cases I know about.

Thanks for looking.

} Could do with playing with, tests, etc., but I think I'll leave it at
} this.

ztie() still calls createspecialhash() which only succeeds if the
parameter is not set.  So to make a ztie parameter local, one has
to do this:

    (){
      local foo
      unset foo
      ztie -d db/gdbm -f gdbmdb foo
      # $foo is now a local ztie
    }

Still, this makes wrong the part of the doc I wrote about it always
creating a global parameter.

Should we document the need to both declare and unset the parameter,
or should we add an implicit unset to ztie() ?

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

[Peter Stephenson]

On Thu, 29 Jan 2015 14:06:25 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> ztie() still calls createspecialhash() which only succeeds if the
> parameter is not set.  So to make a ztie parameter local, one has
> to do this:
> 
>     (){
>       local foo
>       unset foo
>       ztie -d db/gdbm -f gdbmdb foo
>       # $foo is now a local ztie
>     }
> 
> Still, this makes wrong the part of the doc I wrote about it always
> creating a global parameter.
> 
> Should we document the need to both declare and unset the parameter,
> or should we add an implicit unset to ztie() ?

It should probably unset the variable; any other attempt to reuse a
variable declared local (e.g. by declaring it local as a scalar and
assigning it as an array) will silently do that, so I don't see why this
should be different.

Arguably that should be down in createparam(), which isn't told the full
story of the parameter you want to create; stuff like whether it's being
declared local or not, or whether you need to change the type, is
handled up above, creating what's currently a fairly monstrous
interface with createparam() acting as a backend to a lot of half-baked
logic.

pws

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

[Peter Stephenson]

On Fri, 30 Jan 2015 08:59:49 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > Should we document the need to both declare and unset the parameter,
> > or should we add an implicit unset to ztie() ?
> 
> It should probably unset the variable; any other attempt to reuse a
> variable declared local (e.g. by declaring it local as a scalar and
> assigning it as an array) will silently do that, so I don't see why this
> should be different.

I wonder if this is good enough.

pws

diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index a6027de..9896bb5 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -77,8 +77,6 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 	return 1;
     }
 
-    pmname = ztrdup(*args);
-
     resource_name = OPT_ARG(ops, 'f');
 
     dbf = gdbm_open(resource_name, 0, GDBM_WRCREAT | GDBM_SYNC, 0666, 0);
@@ -87,6 +85,21 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 	return 1;
     }
 
+    pmname = ztrdup(*args);
+
+    if ((tied_param = (Param)paramtab->getnode(paramtab, pmname)) &&
+	!(tied_param->node.flags & PM_UNSET)) {
+	/*
+	 * 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.
+	 */
+	if (unsetparam_pm(tied_param, 0, 1)) {
+	    zsfree(pmname);
+	    gdbm_close(dbf);
+	    return 1;
+	}
+    }
     if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys,
 					 PM_REMOVABLE))) {
         zwarnnam(nam, "cannot create the requested parameter %s", pmname);

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

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Jan 30, 2015 11:57 AM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
wrote:
>
> > It should probably unset the variable; any other attempt to reuse a
> > variable declared local (e.g. by declaring it local as a scalar and
> > assigning it as an array) will silently do that, so I don't see why this
> > should be different.
>
> I wonder if this is good enough.

It's actually overkill, it's good enough to simply call unsetparam(pmname)
before the createspecialhash().

If you're actually going to look up the parameter first, you should also
delay the gdbm_open() until you've verified the parameter.

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

[Bart Schaefer]

Ok, some other thoughts on this ...

ztie should have a flag (probably -r) to open the database read-only.
(I've already implemented this, don't go running off to do it.)

However, this raises the question of whether the resulting parameter
should also be readonly.

If it is, then it can't be unset, which currently means it also can't
be zuntied.  On the other hand, even read-only locals disappear when
they go out of scope.

However, this makes me wonder whether it's correct for zuntie to unset
the parameter.  Maybe an explicit zuntie should just convert the param
into an ordinary hash, closing the database file from under it?  (This
I have not implemented yet.)

Thoughts?

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

[Bart Schaefer]

On Jan 30,  7:21pm, Bart Schaefer wrote:
}
} However, this raises the question of whether the resulting parameter
} should also be readonly.

I meant to add:

If it ISN'T, then you can add/change fields of the hash but they are
NOT copied to the database, which could be an interesting feature or
could just be confusing.

Also there's a question of how often the database should be written.
With memory-mapped arrays the writes are effectively instantaneous,
but as presently implemented ztie database files aren't updated until
the parameter is unset.  If this is going to remain this way and the
parameter is not readonly, should failure to write be noisy or silent?

(That question is also affected by the file modes of the database file
if it already exists at the time of ztie-ing.)

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

[Bart Schaefer]

On Jan 30,  7:37pm, Bart Schaefer wrote:
}
} Also there's a question of how often the database should be written.
} With memory-mapped arrays the writes are effectively instantaneous,
} but as presently implemented ztie database files aren't updated until
} the parameter is unset.

I'm wrong about this, the GDBM_SYNC flag is passed to gdbm_open(), so
we do have instantaneous writes.

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

[Bart Schaefer]

Replying to myself with my own conclusions ...

On Jan 30,  7:21pm, Bart Schaefer wrote:
}
} ztie should have a flag (probably -r) to open the database read-only.
} (I've already implemented this, don't go running off to do it.)

Patch below.

} However, this raises the question of whether the resulting parameter
} should also be readonly.

I decided that it should, and added an option "zuntie -u" to forcibly
unset a paramter created in this way.

This also resolves the question of whether writes should silently fail;
they noisily fail.

} However, this makes me wonder whether it's correct for zuntie to unset
} the parameter.  Maybe an explicit zuntie should just convert the param
} into an ordinary hash, closing the database file from under it?  (This
} I have not implemented yet.)

I looked at this, but decided that it is impractical because the ztie'd
parameter reads and writes everything directly from/to the file rather
than caching any of the values in an internal hashtable.

A few other tidbits:

(1) Assigning to a ztie'd hash e.g with DB=( key value ... ) breaks the
ztied property somehow, leaking the GDBM_FILE object.  There must be a
way to fix this, because it doesn't happen for specials like $aliases.

(2) You can get around (1) by using DB+=( key value ... ), but:

(3) There's no operation for bulk deletion of keys from a hash, so to
completely reset the database you need something like

  for k in ${(k)DB}; do unset "DB[$k]"; done

I suspect fixing (1) would also fix (3).

Patch follows, including fixing up the doc but no tests yet.  We don't
have a standard Test/ convention for module tests, other than Test/V*
which is a mixture of tests for module loading in general and for a few
specific modules.  If we were going to add tests for this, where should
they go?


diff --git a/Doc/Zsh/mod_db_gdbm.yo b/Doc/Zsh/mod_db_gdbm.yo
index 6065f86..9097429 100644
--- a/Doc/Zsh/mod_db_gdbm.yo
+++ b/Doc/Zsh/mod_db_gdbm.yo
@@ -11,17 +11,41 @@ The builtins in this module are:
 
 startitem()
 findex(ztie)
-cindex(tied array, creating)
-item(tt(ztie -d db/gdbm -f) var(filename) var(arrayname))(
+cindex(database tied array, creating)
+item(tt(ztie -d db/gdbm -f) var(filename) [ tt(-r) ] var(arrayname))(
 Open the GDBM database identified by var(filename) and, if successful,
-create the associative array var(arrayname) linked to the file.  Note
-that var(arrayname) must be unset at the time tt(ztie) is called, and
-is always created as a global parameter (as if with `tt(typeset -g)').
+create the associative array var(arrayname) linked to the file.  To create
+a local tied array, the parameter must first be declared, so commands
+similar to the following would be executed inside a function scope:
+
+example(local -A sampledb
+ztie -d db/gdbm -f sample.gdbm sampledb)
+
+The tt(-r) option opens the database file for reading only, creating a
+parameter with the readonly attribute.  Without this option, using
+`tt(ztie)' on a file for which the user does not have write permission is
+an error.  If writable, the database is opened synchronously so fields
+changed in var(arrayname) are immediately written to var(filename).
+
+Changes to the file modes var(filename) after it has been opened do not
+alter the state of var(arrayname), but `tt(typeset -r) var(arrayname)'
+works as expected.
 )
 findex(zuntie)
-cindex(tied array, destroying)
-item(tt(zuntie) var(arrayname) ...)(
+cindex(database tied array, destroying)
+item(tt(zuntie) [ tt(-u) ] var(arrayname) ...)(
 Close the GDBM database associated with each var(arrayname) and then
-unset the variable.
+unset the parameter.  The tt(-u) option forces an unset of parameters
+made readonly with `tt(ztie -r)'.
+
+This happens automatically if the parameter is explicitly unset or its
+local scope (function) ends.  Note that a readonly parameter may not be
+explicitly unset, so the only way to unset a global parameter created with
+`tt(ztie -r)' is to use `tt(zuntie -u)'.
 )
 enditem()
+
+The fields of an associative array tied to GDBM are neither cached nor
+otherwise stored in memory, they are read from or written to the database
+on each reference.  Thus, for example, the values in a readonly array may
+be changed by a second writer of the same database file.
diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 9896bb5..a83e32a 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -48,8 +48,8 @@ static const struct gsu_hash gdbm_hash_gsu =
 { hashgetfn, hashsetfn, gdbmhashunsetfn };
 
 static struct builtin bintab[] = {
-    BUILTIN("ztie", 0, bin_ztie, 1, -1, 0, "d:f:", NULL),
-    BUILTIN("zuntie", 0, bin_zuntie, 1, -1, 0, NULL, NULL),
+    BUILTIN("ztie", 0, bin_ztie, 1, -1, 0, "d:f:r", NULL),
+    BUILTIN("zuntie", 0, bin_zuntie, 1, -1, 0, "u", NULL),
 };
 
 /**/
@@ -58,6 +58,7 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 {
     char *resource_name, *pmname;
     GDBM_FILE dbf = NULL;
+    int read_write = GDBM_SYNC, pmflags = PM_REMOVABLE;
     Param tied_param;
 
     if(!OPT_ISSET(ops,'d')) {
@@ -68,6 +69,12 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
         zwarnnam(nam, "you must pass `-f' with a filename", NULL);
 	return 1;
     }
+    if (OPT_ISSET(ops,'r')) {
+	read_write |= GDBM_READER;
+	pmflags |= PM_READONLY;
+    } else {
+	read_write |= GDBM_WRCREAT;
+    }
 
     /* Here should be a lookup of the backend type against
      * a registry.
@@ -79,9 +86,9 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 
     resource_name = OPT_ARG(ops, 'f');
 
-    dbf = gdbm_open(resource_name, 0, GDBM_WRCREAT | GDBM_SYNC, 0666, 0);
+    dbf = gdbm_open(resource_name, 0, read_write, 0666, 0);
     if(!dbf) {
-        zwarnnam(nam, "error opening database file %s", resource_name);
+	zwarnnam(nam, "error opening database file %s", resource_name);
 	return 1;
     }
 
@@ -101,7 +108,7 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
 	}
     }
     if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys,
-					 PM_REMOVABLE))) {
+					 pmflags))) {
         zwarnnam(nam, "cannot create the requested parameter %s", pmname);
 	gdbm_close(dbf);
 	return 1;
@@ -135,6 +142,8 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	queue_signals();
+	if (OPT_ISSET(ops,'u'))
+	    gdbmuntie(pm);	/* clear read-only-ness */
 	if (unsetparam_pm(pm, 0, 1)) {
 	    /* assume already reported */
 	    ret = 1;
@@ -250,19 +259,30 @@ scangdbmkeys(HashTable ht, ScanFunc func, int flags)
 
 /**/
 static void
-gdbmhashunsetfn(Param pm, UNUSED(int exp))
+gdbmuntie(Param pm)
 {
     GDBM_FILE dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
+    HashTable ht = pm->u.hash;
 
-    if (!dbf) /* paranoia */
-	return;
+    if (dbf) /* paranoia */
+	gdbm_close(dbf);
 
-    gdbm_close(dbf);
-    pm->u.hash->tmpdata = NULL;
+    ht->tmpdata = NULL;
 
-    /* hash table is now normal, so proceed normally... */
-    pm->node.flags &= ~PM_SPECIAL;
+    /* for completeness ... createspecialhash() should have an inverse */
+    ht->getnode = ht->getnode2 = gethashnode2;
+    ht->scantab = NULL;
+
+    pm->node.flags &= ~(PM_SPECIAL|PM_READONLY);
     pm->gsu.h = &stdhash_gsu;
+}
+
+/**/
+static void
+gdbmhashunsetfn(Param pm, UNUSED(int exp))
+{
+    gdbmuntie(pm);
+    /* hash table is now normal, so proceed normally... */
     pm->gsu.h->setfn(pm, NULL);
     pm->node.flags |= PM_UNSET;
 }

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

[ZyX]

23.01.2015, 08:20, "Bart Schaefer" <schaefer@brasslantern.com>:
> This module has been sitting around forever, completely undocumented.  The
> basic functions work; I tweaked a couple of things and wrote a short page
> of documentation, which explains about this:

By the way, I just realized that you can write ztie/zuntie on top of zpython. zsh.set_special_hash(varname, hash) does not care which Python object the hash is as long it implements necessary interfaces and it is completely possible (and I think there are already some implementations) to write Python object that will provides access to the database in a needed fashion. It is easier to write database acess code in Python then it is in C, but it will obviously have greater overhead.

if you want to consider merging https://bitbucket.org/ZyX_I/zpython I can convert it back to a patch it used to be once. I introduced it to a list, but it did not get much attention.

>
> schaefer<507> typeset GDBMDB
> schaefer<508> ztie -d db/gdbm -f gdbmdb GDBMDB
> ztie: cannot create the requested parameter GDBMDB
>
> However, there's still this:
>
> schaefer<518> typeset GDBMDB
> schaefer<519> (){ local GDBMDB; unset GDBMDB; ztie -d db/gdbm -f gdbmdb GDBMDB }
>  ../../zsh-5.0/Src/params.c:4914: BUG: in restoring scope of special parameter
> zsh: segmentation fault (core dumped)  Src/zsh
>
> This happens because ztie/zuntie directly manipulate the global paramtab.
> Somebody else may be able to patch that up quicker than I.
>
> diff --git a/Doc/Makefile.in b/Doc/Makefile.in
> index 41af4a3..a420781 100644
> --- a/Doc/Makefile.in
> +++ b/Doc/Makefile.in
> @@ -60,7 +60,7 @@ MODDOCSRC = \
>  Zsh/mod_attr.yo Zsh/mod_cap.yo Zsh/mod_clone.yo \
>  Zsh/mod_compctl.yo Zsh/mod_complete.yo Zsh/mod_complist.yo \
>  Zsh/mod_computil.yo Zsh/mod_curses.yo \
> -Zsh/mod_datetime.yo Zsh/mod_deltochar.yo \
> +Zsh/mod_datetime.yo Zsh/mod_db_gdbm.yo Zsh/mod_deltochar.yo \
>  Zsh/mod_example.yo Zsh/mod_files.yo Zsh/mod_langinfo.yo \
>  Zsh/mod_mapfile.yo Zsh/mod_mathfunc.yo Zsh/mod_newuser.yo \
>  Zsh/mod_parameter.yo Zsh/mod_pcre.yo Zsh/mod_regex.yo \
> diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
> index 9a2a7a5..f079094 100644
> --- a/Src/Modules/db_gdbm.c
> +++ b/Src/Modules/db_gdbm.c
> @@ -39,9 +39,7 @@
>
>  #include <gdbm.h>
>
> -#if 0 /* what is this for? */
>  static char *backtype = "db/gdbm";
> -#endif
>
>  static const struct gsu_scalar gdbm_gsu =
>  { gdbmgetfn, gdbmsetfn, gdbmunsetfn };
> @@ -60,33 +58,38 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
>      Param tied_param;
>
>      if(!OPT_ISSET(ops,'d')) {
> -        zwarnnam(nam, "you must pass `-d db/gdbm' to ztie", NULL);
> +        zwarnnam(nam, "you must pass `-d %s'", backtype);
>          return 1;
>      }
>      if(!OPT_ISSET(ops,'f')) {
> -        zwarnnam(nam, "you must pass `-f' with a filename to ztie", NULL);
> +        zwarnnam(nam, "you must pass `-f' with a filename", NULL);
>          return 1;
>      }
>
>      /* Here should be a lookup of the backend type against
>       * a registry.
>       */
> +    if (strcmp(OPT_ARG(ops, 'd'), backtype) != 0) {
> +        zwarnnam(nam, "unsupported backend type `%s'", OPT_ARG(ops, 'd'));
> + return 1;
> +    }
>
>      pmname = ztrdup(*args);
>
>      resource_name = OPT_ARG(ops, 'f');
>
> -    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
> -        zwarnnam(nam, "cannot create the requested parameter name", NULL);
> - return 1;
> -    }
> -
>      dbf = gdbm_open(resource_name, 0, GDBM_WRCREAT | GDBM_SYNC, 0666, 0);
>      if(!dbf) {
>          zwarnnam(nam, "error opening database file %s", resource_name);
>          return 1;
>      }
>
> +    if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys, 0))) {
> +        zwarnnam(nam, "cannot create the requested parameter %s", pmname);
> + gdbm_close(dbf);
> + return 1;
> +    }
> +
>      tied_param->u.hash->tmpdata = (void *)dbf;
>
>      return 0;
> @@ -98,19 +101,25 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func))
>  {
>      Param pm;
>      GDBM_FILE dbf;
> -
> -    pm = (Param) paramtab->getnode(paramtab, args[0]);
> -    if(!pm) {
> -        zwarnnam(nam, "cannot untie %s", args[0]);
> - return 1;
> +    char *pmname;
> +    int ret = 0;
> +
> +    for (pmname = *args; *args++; pmname = *args) {
> + pm = (Param) paramtab->getnode(paramtab, pmname);
> + if(!pm) {
> +    zwarnnam(nam, "cannot untie %s", pmname);
> +    ret = 1;
> +    continue;
> + }
> +
> + dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
> + gdbm_close(dbf);
> + /* free(pm->u.hash->tmpdata); */
> + pm->u.hash->tmpdata = NULL;
> + paramtab->removenode(paramtab, pm->node.nam);
>      }
>
> -    dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
> -    gdbm_close(dbf);
> -/*    free(pm->u.hash->tmpdata); */
> -    paramtab->removenode(paramtab, pm->node.nam);
> -
> -    return 0;
> +    return ret;
>  }
>
>  /**/

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

[Peter Stephenson]

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.

>} However, this makes me wonder whether it's correct for zuntie to unset
>} the parameter.  Maybe an explicit zuntie should just convert the param
>} into an ordinary hash, closing the database file from under it?  (This
>} I have not implemented yet.)
>
> I looked at this, but decided that it is impractical because the ztie'd
> parameter reads and writes everything directly from/to the file rather
> than caching any of the values in an internal hashtable.

I don't think there's anything wrong with the current behaviour, anyway.

pws

-- 
Peter Stephenson | Principal Engineer Samsung Cambridge Solution Centre
Email: p.stephenson@samsung.com | Phone: +44 1223 434724 |
www.samsung.com

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

[Peter Stephenson]

On Mon, 2 Feb 2015 08:18:29 +0300
ZyX <kp-pav@yandex.ru> wrote:
> if you want to consider merging https://bitbucket.org/ZyX_I/zpython I
> can convert it back to a patch it used to be once. I introduced it to
> a list, but it did not get much attention.

It's clearly a useful addition but I think it would be better to keep it
out of the main source to avoid making that depend (even optionally) on
Python; this is potentially complex for people who don't need it and
subject to change in ways not necessarily tied to changes in zsh, and we
have enough maintenance headaches in the main code that it's nice to
have something that actually does work as an add-on :-)

It would be sensible to provide pointers in the main distribution,
though.

pws

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

[Peter Stephenson]

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);