Re: New functions in sslinfo module

Oh, how can I write a documentation for my functions?

02.07.2014, 16:17, "Воронин Дмитрий" <carriingfate92(at)yandex(dot)ru>:
> 24.06.2014, 00:07, "Andreas Karlsson" <andreas(at)proxel(dot)se>:
>>  On 04/21/2014 07:51 AM, Воронин Дмитрий wrote:
>>>   I put patch generated on git diffs to this letter. I make an a thread in
>>>   postgresql commit fest:
>>>   https://commitfest.postgresql.org/action/patch_view?id=1438
>>  Thanks for the patch, it seems like a useful feature.
>>
>>  === General ===
>>
>>  - Applies cleanly to HEAD and compiles without warnings.
>>
>>  - The test suite passes.
>>
>>  - x509v3 support was added in OpenSSL 0.9.2 and sslinfo already depends
>>  heavily on OpenSSL so no new dependencies.
>>
>>  === User functionality ===
>>
>>  - If you are a user of the sslinfo extension the new functions should be
>>  useful additions.
>>
>>  - I tested the code without SSL, with certificate but without client
>>  certificate, with client certificates first without extensions and the
>>  with. All of this worked fine except for some usability which could be
>>  improved, see below.
>>
>>  - I cannot see the use for ssl_get_count_of_extensions(). When would
>>  anyone need to know the number of extensions? I think this is an
>>  implementation detail of OpenSSL which we do not need to expose. If any
>>  user wants this feature he can count the extension names.
>>
>>  - Documentation is missing for the new functions.
>>
>>  - I think the names of the new functions should be change. Below are my
>>  suggestions. Other suggestions are welcome.
>>
>>  * ssl_extension_value(text)
>>  * ssl_extension_is_critical()
>>  * ssl_extension_names()
>>  * ssl_extension_count() (If we should keep it.)
>>
>>  - Would it be interesting to have a set returning function which returns
>>  all extensions with both the names and the values? Like the below.
>>
>>  $ SELECT * FROM ssl_extensions();
>>          name       |                        value
>>  ------------------+------------------------------------------------------
>>    basicConstraints | CA:FALSE
>>    keyUsage         | Digital Signature, Non Repudiation, Key Encipherment
>>  (2 rows)
>>
>>  - I do not think that ssl_get_extension_names() should return a single
>>  row with NULL when the certificate has no extensions or when there is no
>>  certificate at all. Returning zero rows in this case should make it
>>  easier to use.
>>
>>  - Currently ssl_is_critical_extension() and ssl_get_extension_value()
>>  throw an error when the requested extension is not in the certificate.
>>
>>  I am not sure if I like this behavior. I think I would prefer if the
>>  code always throws an error when name lookup fails, and never when it is
>>  successful. For a valid extension name I think NULL should be returned
>>  when it does not exist.
>>
>>  === Code review: main ===
>>
>>  - Why are X509_NAME_field_to_text(), X509_NAME_to_text(),
>>  ASN1_STRING_to_text() and get_extension() not static? None of these are
>>  a symbol which should be exported.
>>
>>  - I have not worked with extension myself, but since your change adds
>>  functions to the extension I think you need to create a version 1.1
>>  instead of modifying 1.0 in place. If so you also need to write an
>>  upgrade script from 1.0 to 1.1. See dblink--1.0--1.1.sql for an example.
>>
>>  - Please break out the comment fix for ssl_cipher() into a separate patch.
>>
>>  - Why do you use pg_do_encoding_conversion() over pg_any_to_server()?
>>  pg_any_to_server() is implemented using pg_do_encoding_conversion().
>>
>>  - I think you should use OBJ_txt2nid() rather than OBJ_sn2nid() +
>>  OBJ_ln2nid(). You should probably also use OBJ_txt2obj() since
>>  X509_get_ext_by_NID() will call OBJ_nid2obj() anyway.
>>
>>  - You should free the extension_name string. I do not think it is ok to
>>  leak it to the end of the query.
>>
>>  - I think you need to convert the extension values and names to the
>>  server encoding. I just wonder if we need to support data which is
>>  incorrectly encoded.
>>
>>  === Code review: style issues ===
>>
>>  - Trailing whitespace in sslinfo--1.0.sql and sslinfo.c.q
>>
>>  - sslinfo--1.0.sql does not end in a newline.
>>
>>  - I do not think the PostgreSQL project adds authors in the top comment
>>  of files in cases like this. Authors get credit in the commit messages.
>>
>>  - I think you can remove the prototypes of all the ssl_* functions.
>>
>>  - Adding the have in "Indicates whether current client have provided a
>>  certificate" is not necessary. The previous wording looks correct to my
>>  non-native speaker eyes.
>>
>>  - Too much white space in variable declarations in get_extension().
>>
>>  - Extra space before -1 in "X509_get_ext_by_NID(certificate,
>>  extension_nid,  -1);"
>>
>>  - Please do not initialize variables unless necessary. Compilers are
>>  pretty good at warning about uninitialized usage. For example both
>>  locate and extension_nid do not need to be initialized.
>>
>>  - Remove empty line directly before ssl_get_extension_value().
>>
>>  - Try to follow variable naming conventions from other functions (e.g.
>>  use nid rather than extension_nid, membuf rather than bio, sp rather
>>  than value).
>>
>>  - I am pretty sure the variable you call locate should be called
>>  location (or loc for short).
>>
>>  - There should not be any spaces around "->".
>>
>>  - The declaration of *extension in ssl_get_extension_value is not
>>  aligned properly.
>>
>>  - Remove white space in variable declaration in
>>  ssl_get_count_of_extensions().
>>
>>  - Incorrectly alignment of variable declarations in
>>  ssl_get_extension_names().
>>
>>  - All the "Returns X datum" comments look redundant to me, but this is a
>>  matter of preference.
>>
>>  - The star when declaring result in ssl_get_extension_names() should be
>>  put on the other side of the white space.
>>
>>  --
>>  Andreas Karlsson
>
> Hello, Andreas!
>
> I apologize, that I am writing this message today. Thank you for testing my patch!
>
> About user functionality.
> I rename my functions, your suggestions are seemed great. When I wrote those functions, I created names like others in sslinfo extension. OK, I will rename it.
>
> About ssl_get_extension_count(). I will delete this function.
> I will modify functions ssl_extensions(), that it returns a set (key, value). Could you get me an example of code those function?
>
>  >>> - Why are X509_NAME_field_to_text(), X509_NAME_to_text(), ASN1_STRING_to_text() and get_extension() not static? None of these are a symbol which should be exported.
>>>> Why do you use pg_do_encoding_conversion() over pg_any_to_server()? pg_any_to_server() is implemented using pg_do_encoding_conversion().
>
> I don't write a code of those functions and I can't answer on your question.
>
> I will fix your notes and create a new patch version. Thank you.
> --
>
> Best regards, Dmitry Voronin

--
С уважением, Дмитрий Воронин