OSSL_TRACE: enhance documentation and fix doc-nit errors #9224

mspncp · 2019-06-23T17:52:03Z

OSSL_TRACE: enhance documentation and fix doc-nit errors

- Add the following macros to the NAME section:

  - with synopsis
        OSSL_TRACE_CANCEL, OSSL_TRACE, OSSL_TRACE_ENABLED
  - without synopsis
        OSSL_TRACEV (helper macro, not intended for public use)
        OSSL_TRACE[3-8] (omitted on purpose)

- Revise the NOTES section

Checklist

documentation is added or updated (partial fix for issue Newly added undocumented symbols #9095)

mspncp · 2019-06-23T18:02:47Z

doc/man3/OSSL_trace_enabled.pod

 - OpenSSL Tracing API

 =head1 SYNOPSIS

+=for comment generic


Actually, this line disables the synopsis check completely, which is inconvenient when additional symbols are added later on to the pod file. Of course, I can remove the comment line temporarily to get the errors back, but it would be nicer if one could add a specific exclusion list (here: OSSL_TRACEV, OSSL_TRACE3, OSSL_TRACE4, ... OSSL_TRACE8 ).

@levitte do you think this would be a useful feature? If yes, I could raise an issue with a feature request for it.

I have had similar thoughts, but more on a section basis (and we should have our own keyword instead of piggy-backing on comment)

for openssl no-check NAME, SYNOPSIS

I added a feature request in #9226.

mspncp · 2019-06-23T18:05:14Z

doc/man3/OSSL_trace_enabled.pod

@@ -17,7 +23,13 @@ OSSL_TRACE_BEGIN, OSSL_TRACE_END, OSSL_TRACE1, OSSL_TRACE2, OSSL_TRACE9

 /* trace group macros */
 OSSL_TRACE_BEGIN(category) {
-    ...
+     ...


Relative indentation was corrected from +3 to +4 characters.

mspncp · 2019-06-23T18:06:41Z

doc/man3/OSSL_trace_enabled.pod

-        BIO_dump(trace, somememory, somememory_l);
-        OSSL_trace_end(OSSL_TRACE_CATEGORY_TLS, trace);
-    }
+     if (OSSL_trace_enabled(OSSL_TRACE_CATEGORY_TLS)) {


1+4 (one space for perlpod, 4 spaces for C)

I just noticed that the entire NOTES section needs an update. I'll put the pull request in WIP until it's done...

mspncp · 2019-06-23T23:19:24Z

I just noticed that the entire NOTES section needs an update.

Done. Taking the pr out of WIP again.

Please note the new commit message:

    OSSL_TRACE: enhance documentation and fix doc-nit errors
    
    - Add the following macros to the NAME section:
    
      - with synopsis
            OSSL_TRACE_CANCEL, OSSL_TRACE, OSSL_TRACE_ENABLED
      - without synopsis
            OSSL_TRACEV (helper macro, not intended for public use)
            OSSL_TRACE[3-8] (omitted on purpose)
    
    - Revise the NOTES section

- Add the following macros to the NAME section: - with synopsis OSSL_TRACE_CANCEL, OSSL_TRACE, OSSL_TRACE_ENABLED - without synopsis OSSL_TRACEV (helper macro, not intended for public use) OSSL_TRACE[3-8] (omitted on purpose) - Revise the NOTES section

mattcaswell · 2019-06-24T09:59:11Z

doc/man3/OSSL_trace_enabled.pod

@@ -136,26 +152,71 @@ This will normally expand to:
 } while (0);


-C<OSSL_TRACE1()>, ... C<OSSL_TRACE9()> are one-shot macros which essentially wrap
-a single BIO_printf() into a tracing group.
+C<OSSL_TRACE()> and C<OSSL_TRACE1()>, C<OSSL_TRACE1()>, ... C<OSSL_TRACE9()> are


You repeat OSSL_TRACE1 twice here

mattcaswell · 2019-06-24T10:01:24Z

doc/man3/OSSL_trace_enabled.pod

+
+Internally, all one-shot macros are implemented using a generic C<OSSL_TRACEV()>
+macro, since C90 does not support variadic macros. This helper macro has a rather
+weird synopsis and is not intended to be used directly.


I'd make this stronger. Perhaps "must not be used directly".

Hmmm... "must" sounds a bit too strong IMHO. Can we meet in the middle and say "should not be used directly"?

On one hand, I don't want to encourage using OSSL_TRACEV() by documenting it (mainly because of its odd synopsis), but on the other hand, I don't want to forbid it either. One valid use case for an application developer would be for example to add more macros OSSL_TRACE10(), ..., OSSL_TRACE11() along the lines of the existing ones.

(So it's more like "Use it at your own risk. We're unlikely to change it, but we don't advertise it as an official api.")

Its the last bit I was mostly worried about, i.e. people using it and expecting it to be stable. I'd be ok with "should", if there was something about it possibly changing in the future.

This helper macro is really unlikely to change, since the 'numbered' macros derived from it are officially documented and not allowed to change after their initial release. (Currently, the new TRACE api only exists on master.)

I strongly disagree with "must". These are public macros, we cannot dictate their use, only give the users a "fair warning". Our users often use more modern compilers.

Ok, fair enough.

I like how fair we are 😉

mspncp · 2019-06-24T10:43:25Z

Fixup pushed, see 91d0f49.

mspncp · 2019-06-24T13:52:57Z

I pushed one final trivial fixup which I overlooked, see 74ca3d4

mspncp · 2019-06-24T14:28:05Z

Merged to 6f92692, thanks!

- Add the following macros to the NAME section: - with synopsis OSSL_TRACE_CANCEL, OSSL_TRACE, OSSL_TRACE_ENABLED - without synopsis OSSL_TRACEV (helper macro, not intended for public use) OSSL_TRACE[3-8] (omitted on purpose) - Revise the NOTES section Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #9224)

mspncp added the branch: master Merge to master branch label Jun 23, 2019

mspncp mentioned this pull request Jun 23, 2019

Newly added undocumented symbols #9095

Closed

mspncp commented Jun 23, 2019

View reviewed changes

mspncp changed the title ~~OSSL_TRACE: fix doc-nit errors~~ WIP: OSSL_TRACE: fix doc-nit errors Jun 23, 2019

mspncp changed the title ~~WIP: OSSL_TRACE: fix doc-nit errors~~ WIP: OSSL_TRACE: add documentation and fix doc-nit errors Jun 23, 2019

mspncp force-pushed the pr-fix-ossl-trace-doc-nits branch from 6a0fb8a to ef71d83 Compare June 23, 2019 23:16

mspncp changed the title ~~WIP: OSSL_TRACE: add documentation and fix doc-nit errors~~ OSSL_TRACE: enhance documentation and fix doc-nit errors Jun 23, 2019

mspncp requested a review from levitte June 23, 2019 23:22

mspncp force-pushed the pr-fix-ossl-trace-doc-nits branch from ef71d83 to a93dfcd Compare June 23, 2019 23:24

mspncp mentioned this pull request Jun 23, 2019

Add possibility to exclude specific symbols from doc-nits check #9226

Closed

mattcaswell requested changes Jun 24, 2019

View reviewed changes

fixup! OSSL_TRACE: enhance documentation and fix doc-nit errors

91d0f49

levitte approved these changes Jun 24, 2019

View reviewed changes

mspncp added the approval: done This pull request has the required number of approvals label Jun 24, 2019

mattcaswell approved these changes Jun 24, 2019

View reviewed changes

fixup! OSSL_TRACE: enhance documentation and fix doc-nit errors

74ca3d4

levitte approved these changes Jun 24, 2019

View reviewed changes

mspncp closed this Jun 24, 2019

mspncp deleted the pr-fix-ossl-trace-doc-nits branch June 27, 2019 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSSL_TRACE: enhance documentation and fix doc-nit errors #9224

OSSL_TRACE: enhance documentation and fix doc-nit errors #9224

mspncp commented Jun 23, 2019 •

edited

mspncp Jun 23, 2019

levitte Jun 23, 2019

mspncp Jun 23, 2019

mspncp Jun 23, 2019

mspncp Jun 23, 2019

mspncp Jun 23, 2019

mspncp commented Jun 23, 2019 •

edited

mattcaswell Jun 24, 2019

mspncp Jun 24, 2019

mattcaswell Jun 24, 2019

mspncp Jun 24, 2019

mattcaswell Jun 24, 2019

mspncp Jun 24, 2019

levitte Jun 24, 2019

mattcaswell Jun 24, 2019

levitte Jun 24, 2019

mspncp commented Jun 24, 2019 •

edited

mspncp commented Jun 24, 2019

mspncp commented Jun 24, 2019

OSSL_TRACE: enhance documentation and fix doc-nit errors #9224

OSSL_TRACE: enhance documentation and fix doc-nit errors #9224

Conversation

mspncp commented Jun 23, 2019 • edited

Checklist

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

mspncp commented Jun 23, 2019 • edited

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

mspncp commented Jun 24, 2019 • edited

mspncp commented Jun 24, 2019

mspncp commented Jun 24, 2019

mspncp commented Jun 23, 2019 •

edited

mspncp commented Jun 23, 2019 •

edited

mspncp commented Jun 24, 2019 •

edited