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)? 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.) - Julian
