Oh, how can I write a documentation for my functions? 02.07.2014, 16:17, "Воронин Дмитрий" <carriingfat...@yandex.ru>: > 24.06.2014, 00:07, "Andreas Karlsson" <andr...@proxel.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
-- С уважением, Дмитрий Воронин -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
