Re: Changing svn_checksum__from_digest()'s signature

On Wed, Apr 18, 2012 at 8:18 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Hyrum K Wright wrote:
>
>> On Wed, Apr 18, 2012 at 2:15 AM, Julian Foad 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.
>
> It *would* make extending the API easier and localize the 'switch' *if* there were parameterized calls... but there aren't any.  So there is no need for any 'switch' statement, in practice.  Look at the use cases.  There are 14 callers.  The 12 callers outside of checksum.c all want an already-known kind, not a parameterized kind.  The two internal callers each handle any kind, but one of them (_empty_checksum) already contains a switch on the kind.  That leaves only one caller, the implementation of _dup(), where a parameterized API could be beneficial, but such implementation is of course free to use local static functions and other short-cuts.

There aren't any such callers *now*. That does not preclude the
addition of them in the future.

> We could certainly keep the parameterized API for cases when it would be useful, and if it were public that would be the right thing to do, but it's private.
>
>>  What would be the benefit of separate APIs?
>
> Consistency with the style of related APIs is good, granted.  Simplicity
>  of APIs is good too.  Simplicity prefers avoiding possible error
> conditions by design, rather than coding to detect and handle errors.
> Omitting the 'kind' input and the 'error' output (where the only possible error is 'you passed an invalid kind') makes a much simpler API.
>
> In response to consistency: after writing the above I've just realized why this function really is different from the others.  Most of the checksum functions that take a 'kind' input will create whatever kind you like, independently of their other params.  On the other hand, *this* function takes two parameters that are required to agree with each other: the 'digest' parameter has to point to an MD5 digest iff 'kind'=md5, and to a SHA1 digest iff kind=sha1.  It's not like 'const char *digest' is the universal standard external representation of an arbitrary checksum kind; although it would be possible for any checksum kind to have an external repr that can be addressed as 'char *', the next kind we add might prefer to use a different external representation such as 'struct foo'.  So it's not wierd to have one 'checksum' constructor function per checksum kind.  That would be the normal pattern for constructor functions.

That argument makes sense.

-Hyrum

>>>  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/