Wordsmithing and naming consistency #1798

kaduk · 2016-10-28T17:29:27Z

An old wordsmithing patch I had lying around on this branch name, plus some cleanup for the recent BIO rework. Try to make BIO_{read,write}{,ex} and BIO{puts,gets} parameter names match across implementation, declaration, and documentation.

documentation is added or updated
CCLA is signed

levitte · 2016-10-28T18:32:03Z

INSTALL

@@ -806,7 +806,7 @@
                possible to create your own ".conf" and ".tmpl" files and store
                them locally, outside the OpenSSL source tree. This environment
                variable can be set to the directory where these files are held
-                and will have Configure to consider them in addition to the
+                and will have Configure to consider them in preference to the


I disagree, but perhaps it's my english. I understand "in preference" that Configure would look at the indicated local config dir instead of the standard ones. That is not the case.

I don't feel particularly strongly, but "in addition to", to me, indicates that it is appended at the end, so that if something is in the systemwide configuration it takes effect first. Maybe we want something about "override".

Hmm, how about "will have Configure consider them before looking in the standard ones."?

levitte · 2016-10-28T18:33:52Z

doc/man3/BIO_set_callback.pod

@@ -111,39 +111,39 @@ or

 is called before the free operation.

-=item B<BIO_read_ex(b, out, outl, read)>
+=item B<BIO_read_ex(b, data, dlen, read)>


I would replace all occurences of read as parameter with readbytes, just to avoid shadowing read(2) and confusing people...

Looks like I missed that part of the change, as the implementation uses readbytes.

levitte · 2016-10-28T18:37:06Z

include/openssl/bio.h

-int BIO_read_ex(BIO *b, void *data, size_t dlen, size_t *read);
-int BIO_gets(BIO *bp, char *buf, int size);
+int BIO_read_ex(BIO *b, void *data, size_t dlen, size_t *readbytes);
+int BIO_gets(BIO *bp, char *out, int outl);


No, please avoid using out and in in the BIO input and output routines. It's confusing, as to some people (such as myself), out signifies something that goes out (i.e. not something you read) and in signifies something that comes in (i.e. not something you write).

So please revert the BIO_gets() change.

My primary goal was consistency, as the implementation uses out/outl. I'm happy to make it consistent the other way, though.

Please do it the other way around, yes.

levitte · 2016-10-28T18:37:37Z

include/openssl/bio.h

 int BIO_write(BIO *b, const void *data, int dlen);
 int BIO_write_ex(BIO *b, const void *data, size_t dlen, size_t *written);
-int BIO_puts(BIO *bp, const char *buf);
+int BIO_puts(BIO *bp, const char *in);


Please revert the BIO_puts() change. See my explanation at the BIO_gets() change.

Make it clear that the OPENSSL_LOCAL_CONFIG_DIR settings take precedence over the in-tree configs.

kaduk · 2016-10-28T19:54:09Z

(Force-)pushed updated versions of both commits.

levitte · 2016-10-28T20:08:17Z

doc/man3/BIO_read.pod

- int BIO_write(BIO *b, const void *buf, int len);
- int BIO_puts(BIO *b, const char *buf);
+ int BIO_read(BIO *b, void *data, int dlen);
+ int BIO_gets(BIO *b, char *out, int outl);


Oops, didn't notice this one earlier.

levitte · 2016-10-28T20:08:25Z

doc/man3/BIO_read.pod

+ int BIO_read(BIO *b, void *data, int dlen);
+ int BIO_gets(BIO *b, char *out, int outl);
+ int BIO_write(BIO *b, const void *data, int dlen);
+ int BIO_puts(BIO *b, const char *in);


... nor this one

levitte · 2016-10-28T20:09:46Z

doc/man3/BIO_set_callback.pod


 is called before the operation and

- callback_ex(b, BIO_CB_GETS | BIO_CB_RETURN, out, outl, 0, 0L, retvalue, read)
+ callback_ex(b, BIO_CB_GETS | BIO_CB_RETURN, data, dlen, 0, 0L, retvalue, NULL)


NULL? Are you sure about that?

Nope. Too many calls to keep track of, I guess.
(Updates in progress for all mentioned issues.)

levitte · 2016-10-28T20:10:16Z

doc/man3/BIO_set_callback.pod


 after.

-=item B<BIO_puts(b, in)>
+=item B<BIO_puts(b, data)>


Nit-pick: buf

levitte · 2016-10-28T20:10:55Z

doc/man3/BIO_set_callback.pod


 after.

-=item B<BIO_gets(b, out, outl)>
+=item B<BIO_gets(b, data, dlen)>


Nit-pick: buf

levitte · 2016-10-28T20:11:28Z

include/openssl/bio.h

-int BIO_read_ex(BIO *b, void *data, size_t dlen, size_t *read);
-int BIO_gets(BIO *bp, char *buf, int size);
+int BIO_read_ex(BIO *b, void *data, size_t dlen, size_t *readbytes);
+int BIO_gets(BIO *bp, char *data, int dlen);


Again, please reverse

levitte · 2016-10-28T20:11:35Z

include/openssl/bio.h

 int BIO_write(BIO *b, const void *data, int dlen);
 int BIO_write_ex(BIO *b, const void *data, size_t dlen, size_t *written);
-int BIO_puts(BIO *bp, const char *buf);
+int BIO_puts(BIO *bp, const char *data);


... and here

levitte · 2016-10-28T20:13:46Z

Re buf vs data... it seems like we've used buf for the line reading/writing API (gets/puts) while using data for the block read/write API... I realise I argue for keeping that difference, but not strongly so. You decide.

kaduk · 2016-10-28T20:27:00Z

Re buf vs data... it seems like we've used buf for the line reading/writing API (gets/puts) while using data for the block read/write API... I realise I argue for keeping that difference, but not strongly so. You decide.

buf for strings seems reasonable (I'll go with 'size' for the length argument for now) ... and looking at the man page, I still missed several conversions in the previous iteration. So many places to change...

After the recent reworking, not everything matched up, and some comments didn't catch up to the outl-->dlen and inl-->dlen renames that happened during the development of the recent patches. Try to make parameter names consistent across header, implementation, and manual pages. Also remove some trailing whitespace that was inadvertently introduced.

kaduk · 2016-10-28T20:36:45Z

Updated again.
(I still have not figured out when github sends email about new commits, and so err on the side of more notification.)

levitte

Yes, now I'm happy!

levitte · 2016-10-28T21:16:28Z

What's your target? master only or master + 1.1.0 or what? Set the PR lables accordingly.

kaduk · 2016-10-28T21:18:42Z

It looks like the BIO size_t-ification didn't hit 1.1.0, so this should be master-only. (It doesn't look like I have permissions to add/edit labels.)

levitte · 2016-10-28T21:20:01Z

Right. Label added

levitte · 2016-10-28T22:58:11Z

Merged!

Make it clear that the OPENSSL_LOCAL_CONFIG_DIR settings take precedence over the in-tree configs. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1798)

After the recent reworking, not everything matched up, and some comments didn't catch up to the outl-->dlen and inl-->dlen renames that happened during the development of the recent patches. Try to make parameter names consistent across header, implementation, and manual pages. Also remove some trailing whitespace that was inadvertently introduced. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1798)

levitte requested changes Oct 28, 2016

View reviewed changes

Wordsmith INSTALL

de20070

Make it clear that the OPENSSL_LOCAL_CONFIG_DIR settings take precedence over the in-tree configs.

kaduk force-pushed the typo branch from 6b0e0fe to bd7719e Compare October 28, 2016 19:53

levitte requested changes Oct 28, 2016

View reviewed changes

kaduk force-pushed the typo branch from bd7719e to b0d8bb9 Compare October 28, 2016 20:36

levitte approved these changes Oct 28, 2016

View reviewed changes

levitte self-assigned this Oct 28, 2016

levitte added the branch: master Merge to master branch label Oct 28, 2016

levitte added the reviewed label Oct 28, 2016

mattcaswell approved these changes Oct 28, 2016

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Oct 28, 2016

levitte closed this Oct 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wordsmithing and naming consistency #1798

Wordsmithing and naming consistency #1798

kaduk commented Oct 28, 2016

levitte Oct 28, 2016

kaduk Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

kaduk Oct 28, 2016

levitte Oct 28, 2016

kaduk Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

kaduk commented Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

kaduk Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

levitte Oct 28, 2016

levitte commented Oct 28, 2016

kaduk commented Oct 28, 2016

kaduk commented Oct 28, 2016

levitte left a comment

levitte commented Oct 28, 2016

kaduk commented Oct 28, 2016

levitte commented Oct 28, 2016

levitte commented Oct 28, 2016

Wordsmithing and naming consistency #1798

Wordsmithing and naming consistency #1798

Conversation

kaduk commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaduk commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Oct 28, 2016

kaduk commented Oct 28, 2016

kaduk commented Oct 28, 2016

levitte left a comment

Choose a reason for hiding this comment

levitte commented Oct 28, 2016

kaduk commented Oct 28, 2016

levitte commented Oct 28, 2016

levitte commented Oct 28, 2016