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
> +
> + 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?
> + }
> +
> + /* 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.
> + }
> +
> + /* 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?
> +
> + 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.
