Re: svn commit: r1912607 - in /apr/apr/trunk: ./ .github/workflows/ build/ dbm/ include/ include/private/ test/

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 <[email protected]> wrote:
>
> On Fri, Sep 29, 2023 at 3:39 PM <[email protected]> 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