Re: svn commit: r36078 - in trunk/subversion: include/private libsvn_subr

[Ivan Zhakov <iv...@visualsvn.com>]

On Mon, Feb 23, 2009 at 7:34 PM, Greg Stein <gs...@gmail.com> wrote:
> Author: gstein
> Date: Mon Feb 23 08:34:29 2009
> New Revision: 36078
>
> Log:
> Add new binding/retrieval functions to our sqlite interface to deal with
> svn datatypes.
>
[...]
>+svn_error_t *
>+svn_sqlite__column_properties(apr_hash_t **props,
>+                              svn_sqlite__stmt_t *stmt,
>+                              int column,
>+                              apr_pool_t *result_pool,
>+                              apr_pool_t *scratch_pool)

Not related to this commit, but from my experience it's a good
practice add boolean parameter to such database accessors to raise
error on NULL value.

I.e.:
svn_sqlite__column_properties(apr_hash_t **props,
                              svn_sqlite__stmt_t *stmt,
                              int column,
                              svn_boolean_t allow_null,
                              apr_pool_t *result_pool,
                              apr_pool_t *scratch_pool)

I know that NOT NULL are enforcement by sqlite schema, but it's better
to prevent dereference null pointer in our code. Sometimes schema
changes while some of accessors does not updated.

-- 
Ivan Zhakov
VisualSVN Team

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1216967

Re: svn commit: r36078 - in trunk/subversion: include/private libsvn_subr

[Greg Stein <gs...@gmail.com>]

On Mon, Feb 23, 2009 at 23:03, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Mon, Feb 23, 2009 at 7:34 PM, Greg Stein <gs...@gmail.com> wrote:
>> Author: gstein
>> Date: Mon Feb 23 08:34:29 2009
>> New Revision: 36078
>>
>> Log:
>> Add new binding/retrieval functions to our sqlite interface to deal with
>> svn datatypes.
>>
> [...]
>>+svn_error_t *
>>+svn_sqlite__column_properties(apr_hash_t **props,
>>+                              svn_sqlite__stmt_t *stmt,
>>+                              int column,
>>+                              apr_pool_t *result_pool,
>>+                              apr_pool_t *scratch_pool)
>
> Not related to this commit, but from my experience it's a good
> practice add boolean parameter to such database accessors to raise
> error on NULL value.
>
> I.e.:
> svn_sqlite__column_properties(apr_hash_t **props,
>                              svn_sqlite__stmt_t *stmt,
>                              int column,
>                              svn_boolean_t allow_null,
>                              apr_pool_t *result_pool,
>                              apr_pool_t *scratch_pool)
>
> I know that NOT NULL are enforcement by sqlite schema, but it's better
> to prevent dereference null pointer in our code. Sometimes schema
> changes while some of accessors does not updated.

The docstring for the above function:

/* Return the column as a hash of const char * => const svn_string_t *.
   If the column is null, then NULL will be stored into *PROPS. The
   results will be allocated in RESULT_POOL, and any temporary allocations
   will be made in SCRATCH_POOL. */

iow, the function says that a null column is NOT an error, but simply
returns NULL in *props. If the caller cares, then it can raise an
error by looking for that NULL pointer.

As far as "to prevent dereference [of a] null pointer" ... we pass
NULL pointers all the time. And this NULL pointer is part of the
interface contract. I think that callers will be responsible enough to
avoid errors.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1219727