Re: Changing svn_checksum__from_digest()'s signature

On Wed, Apr 18, 2012 at 2:15 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Blair Zajac wrote:
>
>> In case of an illegal svn_checksum_kind_t being passed to
>> svn_checksum__from_digest(), I want to change it from
>>
>> svn_checksum_t *
>> svn_checksum__from_digest(const unsigned char *digest,
>>                           svn_checksum_kind_t kind,
>>                           apr_pool_t *result_pool);
>>
>> to
>>
>> svn_error_t *
>> svn_checksum__from_digest(svn_checksum_t **checksum,
>>                           const unsigned char *digest,
>>                           svn_checksum_kind_t kind,
>>                           apr_pool_t *result_pool);
>
> Wouldn't a better API be:
>
>   svn_checksum_t *
>   svn_checksum__from_digest_md5(digest, result_pool);
>   svn_checksum_t *
>   svn_checksum__from_digest_sha1(digest, result_pool);
>
> since all current callers want just one specific kind (apart from internal calls from the implementations of svn_checksum_dup and svn_checksum_empty_checksum)?

No. This would be the only case where a specific checksum kind is
embedded in the API name rather than as a parameter. The latter makes
extending the API easier in the future, and localizes the 'switch'
statement to that method. What would be the benefit of separate APIs?

> Notice that svn_checksum_empty_checksum() returns NULL if the kind is not recognized, while svn_checksum_dup() does no such check and would pass the unknown kind into ...from_digest().
>
> Therefore it appears that _dup() is the only caller that could have been passing a bad kind -- at least in current trunk code; it may be different in whatever version you're running.  And so, to fix the crash, you will still need to decide how _dup() should handle a bogus checksum struct.  You could have _dup() return NULL, but unless the callers check for null that would just defer the crash.  It would be a little better to call SVN_ERR_MALFUNCTION_NO_RETURN(), to give the client program a little more awareness of the abnormal termination.
>
>
>> This is to prevent a core dump we've observed with our RPC server.
>>
>> The function is in a public .h file but shown as private.  Do I need to add
>> svn_checksum__from_digest2() or can I just change it?
>
> We're allowed to just change it, as I understand it.
>
> (The closest thing I can find in 'hacking' is <http://subversion.apache.org/docs/community-guide/conventions.html#interface-visibility>.  But that doesn't seem to mention the nuances of ABI vs. API and the issue of, IIRC, all symbols in a shared library getting into a Windows DLL ABI even if they're supposed to be private, or whatever exactly that issue was.)

Nobody should be calling this function anyway, so ABI shouldn't be an
issue, IIUC.

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/