On Wed, Apr 18, 2012 at 2:15 AM, Julian Foad <julianf...@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/
