* [PATCH] gdbm bugfixes
@ 2017-06-29 8:32 Sebastian Gniazdowski
2017-06-29 9:08 ` Sebastian Gniazdowski
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Gniazdowski @ 2017-06-29 8:32 UTC (permalink / raw)
To: zsh-workers
[-- Attachment #1: Type: text/plain, Size: 872 bytes --]
Hello,
the "standard" (sent before) fixes, without double-module architecture, just fixes. In the persecution aura around me I better state that the bugs are almost in 100% inherited from old db_gdbm.c:
- no deletehashtable() in custom hash-set-fn
- no emptyhashtable() in custom hash-set-fn
- call to *untie function to remove PM_READONLY flag caused memory leak, the flag is now removed directly, if user gave -u option to untie
- data returned by libgdbm should be free()-d (e.g. key returned by gdbm_nextkey)
- freeparamnode() had if(delunset) condition, this caused memory leak – replaced with myfreeparamnode()
This is effect of many days of work on zredis – I'm saing that the topic is well excavated. Would still hope for two-module architecture, if moddeps problem would be solved.
--
Sebastian Gniazdowski
psprint /at/ zdharma.org
[-- Attachment #2: final_uplift_gdbm.diff --]
[-- Type: application/octet-stream, Size: 12572 bytes --]
diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 0ab0fe7..6369f9e 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -42,7 +42,9 @@ static Param createhash( char *name, int flags );
static int append_tied_name( const char *name );
static int remove_tied_name( const char *name );
static char *unmetafy_zalloc(const char *to_copy, int *new_len);
-static void set_length(char *buf, int size);
+static void myfreeparamnode(HashNode hn);
+
+static int no_database_action = 0;
/*
* Make sure we have all the bits I'm using for memory mapping, otherwise
@@ -72,7 +74,7 @@ static char *backtype = "db/gdbm";
*/
struct gsu_scalar_ext {
- struct gsu_scalar std;
+ struct gsu_scalar std; /* Size of three pointers */
GDBM_FILE dbf;
char *dbfile_path;
};
@@ -106,6 +108,7 @@ static struct paramdef patab[] = {
static int
bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
{
+ struct gsu_scalar_ext *dbf_carrier;
char *resource_name, *pmname;
GDBM_FILE dbf = NULL;
int read_write = GDBM_SYNC, pmflags = PM_REMOVABLE;
@@ -164,21 +167,17 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
if (!(tied_param = createhash(pmname, pmflags))) {
zwarnnam(nam, "cannot create the requested parameter %s", pmname);
- fdtable[gdbm_fdesc(dbf)] = FDT_UNUSED;
gdbm_close(dbf);
return 1;
}
- addmodulefd(gdbm_fdesc(dbf), FDT_MODULE);
- append_tied_name(pmname);
-
tied_param->gsu.h = &gdbm_hash_gsu;
/* Allocate parameter sub-gsu, fill dbf field.
* dbf allocation is 1 to 1 accompanied by
* gsu_scalar_ext allocation. */
- struct gsu_scalar_ext *dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext));
+ dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext));
dbf_carrier->std = gdbm_gsu_ext.std;
dbf_carrier->dbf = dbf;
tied_param->u.hash->tmpdata = (void *)dbf_carrier;
@@ -190,6 +189,10 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
resource_name = xsymlink(resource_name, 1);
}
dbf_carrier->dbfile_path = ztrdup(resource_name);
+
+ addmodulefd(gdbm_fdesc(dbf), FDT_INTERNAL);
+ append_tied_name(pmname);
+
return 0;
}
@@ -215,8 +218,9 @@ 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 (OPT_ISSET(ops,'u')) {
+ pm->node.flags &= ~PM_READONLY;
+ }
if (unsetparam_pm(pm, 0, 1)) {
/* assume already reported */
ret = 1;
@@ -270,8 +274,7 @@ bin_zgdbmpath(char *nam, char **args, Options ops, UNUSED(int func))
*
* It will be left in this state if database doesn't
* contain such key. That might be a drawback, maybe
- * setting to empty value has sense, as no other writer
- * can exist. This would remove subtle hcalloc(1) leak.
+ * setting to empty value has sense.
*/
/**/
@@ -279,7 +282,8 @@ static char *
gdbmgetfn(Param pm)
{
datum key, content;
- int ret;
+ int ret, umlen;
+ char *umkey;
GDBM_FILE dbf;
/* Key already retrieved? There is no sense of asking the
@@ -292,13 +296,13 @@ gdbmgetfn(Param pm)
* - if we are readers, we for sure have newest copy of data
*/
if ( pm->node.flags & PM_UPTODATE ) {
- return pm->u.str ? pm->u.str : (char *) hcalloc(1);
+ return pm->u.str ? pm->u.str : "";
}
/* Unmetafy key. GDBM fits nice into this
* process, as it uses length of data */
- int umlen = 0;
- char *umkey = unmetafy_zalloc(pm->node.nam,¨en);
+ umlen = 0;
+ umkey = unmetafy_zalloc(pm->node.nam,¨en);
key.dptr = umkey;
key.dsize = umlen;
@@ -314,26 +318,26 @@ gdbmgetfn(Param pm)
/* Ensure there's no leak */
if (pm->u.str) {
zsfree(pm->u.str);
+ pm->u.str = NULL;
}
/* Metafy returned data. All fits - metafy
* can obtain data length to avoid using \0 */
pm->u.str = metafy(content.dptr, content.dsize, META_DUP);
+ /* gdbm allocates with malloc */
+ free(content.dptr);
- /* Free key, restoring its original length */
- set_length(umkey, umlen);
- zsfree(umkey);
+ /* Free key */
+ zfree(umkey, umlen+1);
/* Can return pointer, correctly saved inside hash */
return pm->u.str;
}
- /* Free key, restoring its original length */
- set_length(umkey, umlen);
- zsfree(umkey);
+ /* Free key */
+ zfree(umkey, umlen+1);
- /* Can this be "" ? */
- return (char *) hcalloc(1);
+ return "";
}
/**/
@@ -361,7 +365,7 @@ gdbmsetfn(Param pm, char *val)
/* Database */
dbf = ((struct gsu_scalar_ext *)pm->gsu.s)->dbf;
- if (dbf) {
+ if (dbf && no_database_action == 0) {
int umlen = 0;
char *umkey = unmetafy_zalloc(pm->node.nam,¨en);
@@ -378,15 +382,13 @@ gdbmsetfn(Param pm, char *val)
(void)gdbm_store(dbf, key, content, GDBM_REPLACE);
/* Free */
- set_length(umval, umlen);
- zsfree(umval);
+ zfree(umval, umlen+1);
} else {
(void)gdbm_delete(dbf, key);
}
/* Free key */
- set_length(umkey, key.dsize);
- zsfree(umkey);
+ zfree(umkey, key.dsize+1);
}
}
@@ -427,7 +429,7 @@ getgdbmnode(HashTable ht, const char *name)
val_pm = (Param) zshcalloc( sizeof (*val_pm) );
val_pm->node.flags = PM_SCALAR | PM_HASHELEM; /* no PM_UPTODATE */
val_pm->gsu.s = (GsuScalar) ht->tmpdata;
- ht->addnode( ht, ztrdup( name ), val_pm ); // sets pm->node.nam
+ ht->addnode( ht, ztrdup( name ), val_pm ); /* sets pm->node.nam */
}
return (HashNode) val_pm;
@@ -437,7 +439,7 @@ getgdbmnode(HashTable ht, const char *name)
static void
scangdbmkeys(HashTable ht, ScanFunc func, int flags)
{
- datum key;
+ datum key, prev_key;
GDBM_FILE dbf = ((struct gsu_scalar_ext *)ht->tmpdata)->dbf;
/* Iterate keys adding them to hash, so
@@ -456,7 +458,9 @@ scangdbmkeys(HashTable ht, ScanFunc func, int flags)
/* Iterate - no problem as interfacing Param
* will do at most only fetches, not stores */
+ prev_key = key;
key = gdbm_nextkey(dbf, key);
+ free(prev_key.dptr);
}
}
@@ -489,8 +493,15 @@ gdbmhashsetfn(Param pm, HashTable ht)
key = gdbm_firstkey(dbf);
}
- /* just deleted everything, clean up */
- (void)gdbm_reorganize(dbf);
+ /* Just deleted everything, clean up if no new data.
+ * User can also reorganize via gdbmtool. */
+ if (!ht || ht->hsize == 0) {
+ (void)gdbm_reorganize(dbf);
+ }
+
+ no_database_action = 1;
+ emptyhashtable(pm->u.hash);
+ no_database_action = 0;
if (!ht)
return;
@@ -498,9 +509,11 @@ gdbmhashsetfn(Param pm, HashTable ht)
/* Put new strings into database, waiting
* for their interfacing-Params to be created */
- for (i = 0; i < ht->hsize; i++)
+ for (i = 0; i < ht->hsize; i++) {
for (hn = ht->nodes[i]; hn; hn = hn->next) {
struct value v;
+ int umlen = 0;
+ char *umkey, *umval;
v.isarr = v.flags = v.start = 0;
v.end = -1;
@@ -508,8 +521,7 @@ gdbmhashsetfn(Param pm, HashTable ht)
v.pm = (Param) hn;
/* Unmetafy key */
- int umlen = 0;
- char *umkey = unmetafy_zalloc(v.pm->node.nam,¨en);
+ umkey = unmetafy_zalloc(v.pm->node.nam,¨en);
key.dptr = umkey;
key.dsize = umlen;
@@ -517,23 +529,23 @@ gdbmhashsetfn(Param pm, HashTable ht)
queue_signals();
/* Unmetafy */
- char *umval = unmetafy_zalloc(getstrvalue(&v),¨en);
+ umval = unmetafy_zalloc(getstrvalue(&v),¨en);
/* Store */
content.dptr = umval;
content.dsize = umlen;
(void)gdbm_store(dbf, key, content, GDBM_REPLACE);
- /* Free - unmetafy_zalloc allocates exact required
- * space, however unmetafied string can have zeros
- * in content, so we must first fill with non-0 bytes */
- set_length(umval, content.dsize);
- zsfree(umval);
- set_length(umkey, key.dsize);
- zsfree(umkey);
+ /* Free - unmetafy_zalloc allocates
+ * exact required space + 1 null byte */
+ zfree(umval, content.dsize+1);
+ zfree(umkey, key.dsize+1);
unqueue_signals();
}
+ }
+ /* We reuse our hash, the input is to be deleted */
+ deleteparamtable(ht);
}
/**/
@@ -566,14 +578,16 @@ gdbmuntie(Param pm)
static void
gdbmhashunsetfn(Param pm, UNUSED(int exp))
{
+ struct gsu_scalar_ext * gsu_ext;
+
gdbmuntie(pm);
/* Remember custom GSU structure assigned to
* u.hash->tmpdata before hash gets deleted */
- struct gsu_scalar_ext * gsu_ext = pm->u.hash->tmpdata;
+ gsu_ext = pm->u.hash->tmpdata;
- /* Uses normal unsetter. Will delete all owned
- * parameters and also hashtable. */
+ /* Uses normal unsetter (because gdbmuntie is called above).
+ * Will delete all owned field-parameters and also hashtable. */
pm->gsu.h->setfn(pm, NULL);
/* Don't need custom GSU structure with its
@@ -654,13 +668,17 @@ static Param createhash( char *name, int flags ) {
pm->level = locallevel;
/* This creates standard hash. */
- ht = pm->u.hash = newparamtable(32, name);
+ ht = pm->u.hash = newparamtable(17, name);
if (!pm->u.hash) {
paramtab->removenode(paramtab, name);
paramtab->freenode(&pm->node);
- zwarnnam(name, "Out of memory when allocating hash");
+ zwarnnam(name, "out of memory when allocating hash");
+ return NULL;
}
+ /* Does free Param (unsetfn is called) */
+ ht->freenode = myfreeparamnode;
+
/* These provide special features */
ht->getnode = ht->getnode2 = getgdbmnode;
ht->scantab = scangdbmkeys;
@@ -699,6 +717,7 @@ static int append_tied_name( const char *name ) {
static int remove_tied_name( const char *name ) {
int old_len = arrlen(zgdbm_tied);
+ int new_len;
/* Two stage, to always have arrlen() == zfree-size - 1.
* Could do allocation and revert when `not found`, but
@@ -721,13 +740,14 @@ static int remove_tied_name( const char *name ) {
/* Second stage. Size changed? Only old_size-1
* change is possible, but.. paranoia way */
- int new_len = arrlen(zgdbm_tied);
+ new_len = arrlen(zgdbm_tied);
if (new_len != old_len) {
+ char **dst;
char **new_zgdbm_tied = zshcalloc((new_len+1) * sizeof(char *));
/* Copy */
p = zgdbm_tied;
- char **dst = new_zgdbm_tied;
+ dst = new_zgdbm_tied;
while (*p) {
*dst++ = *p++;
}
@@ -746,10 +766,9 @@ static int remove_tied_name( const char *name ) {
* - duplicates bufer to work on it,
* - does zalloc of exact size for the new string,
* - restores work buffer to original content, to restore strlen
- *
- * No zsfree()-confusing string will be produced.
*/
-static char *unmetafy_zalloc(const char *to_copy, int *new_len) {
+static char *
+unmetafy_zalloc(const char *to_copy, int *new_len) {
char *work, *to_return;
int my_new_len = 0;
@@ -771,16 +790,27 @@ static char *unmetafy_zalloc(const char *to_copy, int *new_len) {
return to_return;
}
-/*
- * For zsh-allocator, rest of Zsh seems to use
- * free() instead of zsfree(), and such length
- * restoration causes slowdown, but all is this
- * way strict - correct */
-static void set_length(char *buf, int size) {
- buf[size]='\0';
- while ( -- size >= 0 ) {
- buf[size]=' ';
+static void
+myfreeparamnode(HashNode hn)
+{
+ Param pm = (Param) hn;
+
+ /* Upstream: The second argument of unsetfn() is used by modules to
+ * differentiate "exp"licit unset from implicit unset, as when
+ * a parameter is going out of scope. It's not clear which
+ * of these applies here, but passing 1 has always worked.
+ */
+
+ /* if (delunset) */
+ pm->gsu.s->unsetfn(pm, 1);
+
+ zsfree(pm->node.nam);
+ /* If this variable was tied by the user, ename was ztrdup'd */
+ if (pm->node.flags & PM_TIED && pm->ename) {
+ zsfree(pm->ename);
+ pm->ename = NULL;
}
+ zfree(pm, sizeof(struct param));
}
#else
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gdbm bugfixes
2017-06-29 8:32 [PATCH] gdbm bugfixes Sebastian Gniazdowski
@ 2017-06-29 9:08 ` Sebastian Gniazdowski
2017-06-29 9:18 ` Peter Stephenson
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Gniazdowski @ 2017-06-29 9:08 UTC (permalink / raw)
To: zsh-workers
[-- Attachment #1: Type: text/plain, Size: 191 bytes --]
PS. Missed one C++ comment, it's removed in current patch
On 29.06.2017 at 10:32:16, Sebastian Gniazdowski (psprint@zdharma.org) wrote:
--
Sebastian Gniazdowski
psprint /at/ zdharma.org
[-- Attachment #2: final_uplift_gdbm2.diff --]
[-- Type: application/octet-stream, Size: 13024 bytes --]
diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 0ab0fe7..cf13224 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -42,7 +42,9 @@ static Param createhash( char *name, int flags );
static int append_tied_name( const char *name );
static int remove_tied_name( const char *name );
static char *unmetafy_zalloc(const char *to_copy, int *new_len);
-static void set_length(char *buf, int size);
+static void myfreeparamnode(HashNode hn);
+
+static int no_database_action = 0;
/*
* Make sure we have all the bits I'm using for memory mapping, otherwise
@@ -72,7 +74,7 @@ static char *backtype = "db/gdbm";
*/
struct gsu_scalar_ext {
- struct gsu_scalar std;
+ struct gsu_scalar std; /* Size of three pointers */
GDBM_FILE dbf;
char *dbfile_path;
};
@@ -106,6 +108,7 @@ static struct paramdef patab[] = {
static int
bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
{
+ struct gsu_scalar_ext *dbf_carrier;
char *resource_name, *pmname;
GDBM_FILE dbf = NULL;
int read_write = GDBM_SYNC, pmflags = PM_REMOVABLE;
@@ -164,21 +167,17 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
if (!(tied_param = createhash(pmname, pmflags))) {
zwarnnam(nam, "cannot create the requested parameter %s", pmname);
- fdtable[gdbm_fdesc(dbf)] = FDT_UNUSED;
gdbm_close(dbf);
return 1;
}
- addmodulefd(gdbm_fdesc(dbf), FDT_MODULE);
- append_tied_name(pmname);
-
tied_param->gsu.h = &gdbm_hash_gsu;
/* Allocate parameter sub-gsu, fill dbf field.
* dbf allocation is 1 to 1 accompanied by
* gsu_scalar_ext allocation. */
- struct gsu_scalar_ext *dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext));
+ dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext));
dbf_carrier->std = gdbm_gsu_ext.std;
dbf_carrier->dbf = dbf;
tied_param->u.hash->tmpdata = (void *)dbf_carrier;
@@ -190,6 +189,10 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
resource_name = xsymlink(resource_name, 1);
}
dbf_carrier->dbfile_path = ztrdup(resource_name);
+
+ addmodulefd(gdbm_fdesc(dbf), FDT_INTERNAL);
+ append_tied_name(pmname);
+
return 0;
}
@@ -215,8 +218,9 @@ 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 (OPT_ISSET(ops,'u')) {
+ pm->node.flags &= ~PM_READONLY;
+ }
if (unsetparam_pm(pm, 0, 1)) {
/* assume already reported */
ret = 1;
@@ -270,8 +274,7 @@ bin_zgdbmpath(char *nam, char **args, Options ops, UNUSED(int func))
*
* It will be left in this state if database doesn't
* contain such key. That might be a drawback, maybe
- * setting to empty value has sense, as no other writer
- * can exist. This would remove subtle hcalloc(1) leak.
+ * setting to empty value has sense.
*/
/**/
@@ -279,7 +282,8 @@ static char *
gdbmgetfn(Param pm)
{
datum key, content;
- int ret;
+ int ret, umlen;
+ char *umkey;
GDBM_FILE dbf;
/* Key already retrieved? There is no sense of asking the
@@ -292,13 +296,13 @@ gdbmgetfn(Param pm)
* - if we are readers, we for sure have newest copy of data
*/
if ( pm->node.flags & PM_UPTODATE ) {
- return pm->u.str ? pm->u.str : (char *) hcalloc(1);
+ return pm->u.str ? pm->u.str : "";
}
/* Unmetafy key. GDBM fits nice into this
* process, as it uses length of data */
- int umlen = 0;
- char *umkey = unmetafy_zalloc(pm->node.nam,¨en);
+ umlen = 0;
+ umkey = unmetafy_zalloc(pm->node.nam,¨en);
key.dptr = umkey;
key.dsize = umlen;
@@ -314,26 +318,26 @@ gdbmgetfn(Param pm)
/* Ensure there's no leak */
if (pm->u.str) {
zsfree(pm->u.str);
+ pm->u.str = NULL;
}
/* Metafy returned data. All fits - metafy
* can obtain data length to avoid using \0 */
pm->u.str = metafy(content.dptr, content.dsize, META_DUP);
+ /* gdbm allocates with malloc */
+ free(content.dptr);
- /* Free key, restoring its original length */
- set_length(umkey, umlen);
- zsfree(umkey);
+ /* Free key */
+ zfree(umkey, umlen+1);
/* Can return pointer, correctly saved inside hash */
return pm->u.str;
}
- /* Free key, restoring its original length */
- set_length(umkey, umlen);
- zsfree(umkey);
+ /* Free key */
+ zfree(umkey, umlen+1);
- /* Can this be "" ? */
- return (char *) hcalloc(1);
+ return "";
}
/**/
@@ -361,7 +365,7 @@ gdbmsetfn(Param pm, char *val)
/* Database */
dbf = ((struct gsu_scalar_ext *)pm->gsu.s)->dbf;
- if (dbf) {
+ if (dbf && no_database_action == 0) {
int umlen = 0;
char *umkey = unmetafy_zalloc(pm->node.nam,¨en);
@@ -378,15 +382,13 @@ gdbmsetfn(Param pm, char *val)
(void)gdbm_store(dbf, key, content, GDBM_REPLACE);
/* Free */
- set_length(umval, umlen);
- zsfree(umval);
+ zfree(umval, umlen+1);
} else {
(void)gdbm_delete(dbf, key);
}
/* Free key */
- set_length(umkey, key.dsize);
- zsfree(umkey);
+ zfree(umkey, key.dsize+1);
}
}
@@ -427,7 +429,7 @@ getgdbmnode(HashTable ht, const char *name)
val_pm = (Param) zshcalloc( sizeof (*val_pm) );
val_pm->node.flags = PM_SCALAR | PM_HASHELEM; /* no PM_UPTODATE */
val_pm->gsu.s = (GsuScalar) ht->tmpdata;
- ht->addnode( ht, ztrdup( name ), val_pm ); // sets pm->node.nam
+ ht->addnode( ht, ztrdup( name ), val_pm ); /* sets pm->node.nam */
}
return (HashNode) val_pm;
@@ -437,7 +439,7 @@ getgdbmnode(HashTable ht, const char *name)
static void
scangdbmkeys(HashTable ht, ScanFunc func, int flags)
{
- datum key;
+ datum key, prev_key;
GDBM_FILE dbf = ((struct gsu_scalar_ext *)ht->tmpdata)->dbf;
/* Iterate keys adding them to hash, so
@@ -456,7 +458,9 @@ scangdbmkeys(HashTable ht, ScanFunc func, int flags)
/* Iterate - no problem as interfacing Param
* will do at most only fetches, not stores */
+ prev_key = key;
key = gdbm_nextkey(dbf, key);
+ free(prev_key.dptr);
}
}
@@ -489,8 +493,15 @@ gdbmhashsetfn(Param pm, HashTable ht)
key = gdbm_firstkey(dbf);
}
- /* just deleted everything, clean up */
- (void)gdbm_reorganize(dbf);
+ /* Just deleted everything, clean up if no new data.
+ * User can also reorganize via gdbmtool. */
+ if (!ht || ht->hsize == 0) {
+ (void)gdbm_reorganize(dbf);
+ }
+
+ no_database_action = 1;
+ emptyhashtable(pm->u.hash);
+ no_database_action = 0;
if (!ht)
return;
@@ -498,9 +509,11 @@ gdbmhashsetfn(Param pm, HashTable ht)
/* Put new strings into database, waiting
* for their interfacing-Params to be created */
- for (i = 0; i < ht->hsize; i++)
+ for (i = 0; i < ht->hsize; i++) {
for (hn = ht->nodes[i]; hn; hn = hn->next) {
struct value v;
+ int umlen = 0;
+ char *umkey, *umval;
v.isarr = v.flags = v.start = 0;
v.end = -1;
@@ -508,8 +521,7 @@ gdbmhashsetfn(Param pm, HashTable ht)
v.pm = (Param) hn;
/* Unmetafy key */
- int umlen = 0;
- char *umkey = unmetafy_zalloc(v.pm->node.nam,¨en);
+ umkey = unmetafy_zalloc(v.pm->node.nam,¨en);
key.dptr = umkey;
key.dsize = umlen;
@@ -517,23 +529,23 @@ gdbmhashsetfn(Param pm, HashTable ht)
queue_signals();
/* Unmetafy */
- char *umval = unmetafy_zalloc(getstrvalue(&v),¨en);
+ umval = unmetafy_zalloc(getstrvalue(&v),¨en);
/* Store */
content.dptr = umval;
content.dsize = umlen;
(void)gdbm_store(dbf, key, content, GDBM_REPLACE);
- /* Free - unmetafy_zalloc allocates exact required
- * space, however unmetafied string can have zeros
- * in content, so we must first fill with non-0 bytes */
- set_length(umval, content.dsize);
- zsfree(umval);
- set_length(umkey, key.dsize);
- zsfree(umkey);
+ /* Free - unmetafy_zalloc allocates
+ * exact required space + 1 null byte */
+ zfree(umval, content.dsize+1);
+ zfree(umkey, key.dsize+1);
unqueue_signals();
}
+ }
+ /* We reuse our hash, the input is to be deleted */
+ deleteparamtable(ht);
}
/**/
@@ -566,14 +578,16 @@ gdbmuntie(Param pm)
static void
gdbmhashunsetfn(Param pm, UNUSED(int exp))
{
+ struct gsu_scalar_ext * gsu_ext;
+
gdbmuntie(pm);
/* Remember custom GSU structure assigned to
* u.hash->tmpdata before hash gets deleted */
- struct gsu_scalar_ext * gsu_ext = pm->u.hash->tmpdata;
+ gsu_ext = pm->u.hash->tmpdata;
- /* Uses normal unsetter. Will delete all owned
- * parameters and also hashtable. */
+ /* Uses normal unsetter (because gdbmuntie is called above).
+ * Will delete all owned field-parameters and also hashtable. */
pm->gsu.h->setfn(pm, NULL);
/* Don't need custom GSU structure with its
@@ -654,13 +668,17 @@ static Param createhash( char *name, int flags ) {
pm->level = locallevel;
/* This creates standard hash. */
- ht = pm->u.hash = newparamtable(32, name);
+ ht = pm->u.hash = newparamtable(17, name);
if (!pm->u.hash) {
paramtab->removenode(paramtab, name);
paramtab->freenode(&pm->node);
- zwarnnam(name, "Out of memory when allocating hash");
+ zwarnnam(name, "out of memory when allocating hash");
+ return NULL;
}
+ /* Does free Param (unsetfn is called) */
+ ht->freenode = myfreeparamnode;
+
/* These provide special features */
ht->getnode = ht->getnode2 = getgdbmnode;
ht->scantab = scangdbmkeys;
@@ -699,6 +717,7 @@ static int append_tied_name( const char *name ) {
static int remove_tied_name( const char *name ) {
int old_len = arrlen(zgdbm_tied);
+ int new_len;
/* Two stage, to always have arrlen() == zfree-size - 1.
* Could do allocation and revert when `not found`, but
@@ -721,13 +740,14 @@ static int remove_tied_name( const char *name ) {
/* Second stage. Size changed? Only old_size-1
* change is possible, but.. paranoia way */
- int new_len = arrlen(zgdbm_tied);
+ new_len = arrlen(zgdbm_tied);
if (new_len != old_len) {
+ char **dst;
char **new_zgdbm_tied = zshcalloc((new_len+1) * sizeof(char *));
/* Copy */
p = zgdbm_tied;
- char **dst = new_zgdbm_tied;
+ dst = new_zgdbm_tied;
while (*p) {
*dst++ = *p++;
}
@@ -746,10 +766,9 @@ static int remove_tied_name( const char *name ) {
* - duplicates bufer to work on it,
* - does zalloc of exact size for the new string,
* - restores work buffer to original content, to restore strlen
- *
- * No zsfree()-confusing string will be produced.
*/
-static char *unmetafy_zalloc(const char *to_copy, int *new_len) {
+static char *
+unmetafy_zalloc(const char *to_copy, int *new_len) {
char *work, *to_return;
int my_new_len = 0;
@@ -761,7 +780,7 @@ static char *unmetafy_zalloc(const char *to_copy, int *new_len) {
/* This string can be correctly zsfree()-d */
to_return = (char *) zalloc((my_new_len+1)*sizeof(char));
- memcpy(to_return, work, sizeof(char)*my_new_len); // memcpy handles $'\0'
+ memcpy(to_return, work, sizeof(char)*my_new_len); /* memcpy handles $'\0' */
to_return[my_new_len]='\0';
/* Restore original strlen and correctly free */
@@ -771,16 +790,27 @@ static char *unmetafy_zalloc(const char *to_copy, int *new_len) {
return to_return;
}
-/*
- * For zsh-allocator, rest of Zsh seems to use
- * free() instead of zsfree(), and such length
- * restoration causes slowdown, but all is this
- * way strict - correct */
-static void set_length(char *buf, int size) {
- buf[size]='\0';
- while ( -- size >= 0 ) {
- buf[size]=' ';
+static void
+myfreeparamnode(HashNode hn)
+{
+ Param pm = (Param) hn;
+
+ /* Upstream: The second argument of unsetfn() is used by modules to
+ * differentiate "exp"licit unset from implicit unset, as when
+ * a parameter is going out of scope. It's not clear which
+ * of these applies here, but passing 1 has always worked.
+ */
+
+ /* if (delunset) */
+ pm->gsu.s->unsetfn(pm, 1);
+
+ zsfree(pm->node.nam);
+ /* If this variable was tied by the user, ename was ztrdup'd */
+ if (pm->node.flags & PM_TIED && pm->ename) {
+ zsfree(pm->ename);
+ pm->ename = NULL;
}
+ zfree(pm, sizeof(struct param));
}
#else
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gdbm bugfixes
2017-06-29 9:08 ` Sebastian Gniazdowski
@ 2017-06-29 9:18 ` Peter Stephenson
0 siblings, 0 replies; 3+ messages in thread
From: Peter Stephenson @ 2017-06-29 9:18 UTC (permalink / raw)
To: zsh-workers
I will commit this unless anyone comments or commits it first...
pws
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-29 9:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 8:32 [PATCH] gdbm bugfixes Sebastian Gniazdowski
2017-06-29 9:08 ` Sebastian Gniazdowski
2017-06-29 9:18 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).