Hello Yann, based on your email I created new PR which incorporates your proposed changes. Feel free to review it.
https://github.com/apache/apr/pull/49 On Mon, Oct 2, 2023 at 2:32 PM Yann Ylavic <ylavic....@gmail.com> wrote: > > On Fri, Sep 29, 2023 at 3:39 PM <jor...@apache.org> wrote: > > > > Author: jorton > > Date: Fri Sep 29 13:39:11 2023 > > New Revision: 1912607 > [] > > --- apr/apr/trunk/dbm/apr_dbm_lmdb.c (added) > > +++ apr/apr/trunk/dbm/apr_dbm_lmdb.c Fri Sep 29 13:39:11 2023 > [] > > +static apr_status_t vt_lmdb_open(apr_dbm_t **pdb, const char *pathname, > > + apr_int32_t mode, apr_fileperms_t perm, > > + apr_pool_t *pool) > > +{ > [] > > + { > > + int dberr; > > + file.txn = NULL; > > + file.cursor = NULL; > > + > > + if ((dberr = mdb_env_create(&file.env)) == 0) { > > + //XXX: properly set db size > > + if ((dberr = mdb_env_set_mapsize(file.env, UINT32_MAX)) == 0){ > > + if ((dberr = mdb_env_open(file.env, pathname, dbmode | > > DEFAULT_ENV_FLAGS, apr_posix_perms2mode(perm))) == 0) { > > + if ((dberr = mdb_txn_begin(file.env, NULL, dbmode, > > &file.txn)) == 0){ > > + if ((dberr = mdb_dbi_open(file.txn, NULL, > > dbi_open_flags, &file.dbi)) != 0){ > > + /* close the env handler */ > > + mdb_env_close(file.env); > > + } > > + } > > + } > > + } > > + } > > + > > + if (truncate){ > > + if ((dberr = mdb_drop(file.txn, file.dbi, 0)) != 0){ > > + mdb_env_close(file.env); > > + } > > + } > > + > > + if (dberr != 0) > > + return db2s(dberr); > > + } > > This seems to potentially double-close or leak "file.env" on some > errors, maybe write it like the below instead? > > dberr = mdb_env_create(&file.env); > if (dberr != 0) { > return db2s(dberr); > } > dberr = ...; > if (dberr == 0) { > dberr = ...; > } > dberr = ...; > if (dberr == 0) { > dberr = ...; > } > ... > if (dberr == 0) { > dberr = mdb_dbi_open(file.txn, NULL, dbi_open_flags, &file.dbi); > if (dberr == 0 && truncate) { > dberr = mdb_drop(file.txn, file.dbi, 0); > if (dberr != 0) { > mdb_dbi_close(file.env, file.dbi); > } > } > } > if (dberr != 0) { > mdb_env_close(file.env); > return db2s(dberr); > } > > > + > > + /* we have an open database... return it */ > > + *pdb = apr_pcalloc(pool, sizeof(**pdb)); > > + (*pdb)->pool = pool; > > + (*pdb)->type = &apr_dbm_type_lmdb; > > + (*pdb)->file = apr_pmemdup(pool, &file, sizeof(file)); > > + > > + /* ### register a cleanup to close the DBM? */ > > + > > + return APR_SUCCESS; > > +} > > + > > +static void vt_lmdb_close(apr_dbm_t *dbm) > > +{ > > + real_file_t *f = dbm->file; > > + > > + if (f->cursor){ > > + mdb_cursor_close(f->cursor); > > + f->cursor = NULL; > > + } > > + > > + if (f->txn){ > > + mdb_txn_commit(f->txn); > > + f->txn = NULL; > > + } > > Shouldn't we abort the transaction here if there was no explicit > commit (store/del) previously? > In any case, the docs[1] for commit/abort seem to say that both > invalidate the ->cursor, so maybe we want to write this like: > > if (f->txn){ > mdb_txn_{commit,abort}(f->txn); > f->txn = NULL; > f->cursor = NULL; > } > if (f->cursor){ > mdb_cursor_close(f->cursor); > f->cursor = NULL; > } > > ? > [1] > http://www.lmdb.tech/doc/group__mdb.html#ga846fbd6f46105617ac9f4d76476f6597 I think we should commit any pending transactions in vt_lmdb_close(), therefore I used mdb_txn_commit(). If you think I should use abort instead, please let me know. I also reorganized the order as you correctly pointed out. > > > + > > + mdb_dbi_close(f->env, f->dbi); > > + mdb_env_close(f->env); > > + > > + f->env = NULL; > > + f->dbi = 0; > > +} > [] > > +static apr_status_t vt_lmdb_store(apr_dbm_t *dbm, apr_datum_t key, > > + apr_datum_t value) > > +{ > > + real_file_t *f = dbm->file; > > + int rv; > > + MDB_val ckey = { 0 }; > > + MDB_val cvalue = { 0 }; > > + > > + ckey.mv_data = key.dptr; > > + ckey.mv_size = key.dsize; > > + > > + cvalue.mv_data = value.dptr; > > + cvalue.mv_size = value.dsize; > > + > > + if ((rv = mdb_put(f->txn, f->dbi, &ckey, &cvalue, 0)) == 0) { > > + /* commit transaction */ > > + if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) > > + && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == > > MDB_SUCCESS)) { > > + f->cursor = NULL; > > + } > > AIUI, f->cursor should be reset whether or not mdb_txn_begin() > succeeds here, and f->txn if any of _commit() or _begin() fails? You are right. IMHO, if mdb_txn_commit fails, we should not reset f->cursor since we might want to store the value again (e.g. in case of not enough space on disk?). But if mdb_txn_commit succeeds, we can reset f->cursor to NULL. > > > + } > > + > > + /* store any error info into DBM, and return a status code. */ > > + return set_error(dbm, rv); > > +} > > + > > +static apr_status_t vt_lmdb_del(apr_dbm_t *dbm, apr_datum_t key) > > +{ > > + real_file_t *f = dbm->file; > > + int rv; > > + MDB_val ckey = { 0 }; > > + > > + ckey.mv_data = key.dptr; > > + ckey.mv_size = key.dsize; > > + > > + if ((rv = mdb_del(f->txn, f->dbi, &ckey, NULL)) == 0) { > > + /* commit transaction */ > > + if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) > > + && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == > > MDB_SUCCESS)) { > > + f->cursor = NULL; > > + } > > Likewise for f->cursor and f->txn resetting. Fixed too, thanks! > > > + } > > + > > + /* store any error info into DBM, and return a status code. */ > > + return set_error(dbm, rv); > > +} > [] > > +static apr_status_t vt_lmdb_firstkey(apr_dbm_t *dbm, apr_datum_t * pkey) > > +{ > > + real_file_t *f = dbm->file; > > + MDB_val first, data; > > + int dberr; > > + > > + if ((dberr = mdb_cursor_open(f->txn, f->dbi, &f->cursor)) == 0) { > > + dberr = mdb_cursor_get(f->cursor, &first, &data, MDB_FIRST); > > + if (dberr == MDB_NOTFOUND) { > > + memset(&first, 0, sizeof(first)); > > + mdb_cursor_close(f->cursor); > > + f->cursor = NULL; > > + dberr = 0; > > + } > > + } > > Clear/memset "first" if mdb_cursor_open() fails too? This makes sense, I will fix it. > > > + > > + pkey->dptr = first.mv_data; > > + pkey->dsize = first.mv_size; > > + > > + /* store any error info into DBM, and return a status code. */ > > + return set_error(dbm, dberr); > > +} > > + > > +static apr_status_t vt_lmdb_nextkey(apr_dbm_t *dbm, apr_datum_t * pkey) > > +{ > > + real_file_t *f = dbm->file; > > + MDB_val ckey, data; > > + int dberr; > > + > > + ckey.mv_data = pkey->dptr; > > + ckey.mv_size = pkey->dsize; > > + > > + if (f->cursor == NULL){ > > + return APR_EINVAL; > > + } > > + > > + dberr = mdb_cursor_get(f->cursor, &ckey, &data, MDB_NEXT); > > + if (dberr == MDB_NOTFOUND) { > > + mdb_cursor_close(f->cursor); > > + f->cursor = NULL; > > + dberr = 0; > > + ckey.mv_data = NULL; > > + ckey.mv_size = 0; > > + } > > + > > + pkey->dptr = ckey.mv_data; > > + pkey->dsize = ckey.mv_size; > > + > > + /* store any error info into DBM, and return a status code. */ > > + return set_error(dbm, dberr); > > +} > > > Regards; > Yann. > -- Regards Luboš Uhliarik
