Hi,

Jack Burton wrote on Wed, Jun 21, 2017 at 11:45:38PM +0930:

> X509_VERIFY_PARAM_set_flags(3) states that
> X509_VERIFY_PARAM_set_flags() and X509_VERIFY_PARAM_clear_flags()
> both "return 1 for success or 0 for failure".

Which is true.

> But both those functions always return 1

In the current implementation.

> The trivial diff below amends the man page to reflect reality.

No, the diff is not needed to make the documentation match reality.
The documentation already matches reality.

Instead, the diff constitutes a change of the public interface.

The documentation of a library function does not only state
what the current implementation does.  Above all, it tells
users of the public interface which return values they must
expect, including in future versions of the library.

So what Stephen Henson did here when writing the documentation
was reserving the right to implement functionality in the future
that might possibly cause these functions to fail.

> I'm wondering whether it's perhaps worth changing the type of
> those two functions

Hell no, what a terrible idea.  Changing the prototype of a public
interface in a library requires a major bump of the library version
and potentially requires changing all application code that uses
the interface.

With the proposed change, you reward reckless or lazy programmers
who ignored the return value of these functions in their code
and you punish programmers who carefully coded according to spec -
because after your change, error handling code for failure of
these functions will no longer compile.

So even when you have strong reasons to change functionality in a
library, you generally try to avoid changing the prototypes of
established interfaces.


All that said, it may or may not have been a poor design decision
to make these functions return int rather than void, and i have
no idea whether anything that could cause them to fail might ever
be useful - but that ship has sailed, the interfaces are now public
and you need very strong reasons to change them.

Yours,
  Ingo


> Index: man/X509_VERIFY_PARAM_set_flags.3
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/man/X509_VERIFY_PARAM_set_flags.3,v
> retrieving revision 1.5
> diff -u -p -r1.5 X509_VERIFY_PARAM_set_flags.3
> --- man/X509_VERIFY_PARAM_set_flags.3 6 Jan 2017 21:30:27
> -0000 1.5 +++ man/X509_VERIFY_PARAM_set_flags.3       21 Jun
> 2017 13:29:13 -0000 @@ -183,8 +183,11 @@ sets the maximum verification
> depth to That is the maximum number of untrusted CA certificates that
> can appear in a chain.
>  .Sh RETURN VALUES
> -.Fn X509_VERIFY_PARAM_set_flags ,
> -.Fn X509_VERIFY_PARAM_clear_flags ,
> +.Fn X509_VERIFY_PARAM_set_flags
> +and
> +.Fn X509_VERIFY_PARAM_clear_flags
> +always return 1.
> +.Pp
>  .Fn X509_VERIFY_PARAM_set_purpose ,
>  .Fn X509_VERIFY_PARAM_set_trust ,
>  .Fn X509_VERIFY_PARAM_add0_policy ,

Reply via email to