Skip to content

Add man 7 page about properties.#9011

Closed
paulidale wants to merge 5 commits intoopenssl:masterfrom
paulidale:property-doc
Closed

Add man 7 page about properties.#9011
paulidale wants to merge 5 commits intoopenssl:masterfrom
paulidale:property-doc

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented May 27, 2019

Add a page about properties in the man7 section of the public documentation.

  • documentation is added or updated

@paulidale paulidale mentioned this pull request May 27, 2019
Closed
1 task
@paulidale paulidale added branch: master Applies to master branch Core and removed Core labels May 27, 2019
levitte
levitte requested changes May 27, 2019
View reviewed changes
@paulidale
Copy link
Contributor Author

paulidale commented May 27, 2019

Comments addressed (except for FIPS vs fips).

@levitte
Copy link
Member

levitte commented May 27, 2019

Looks good to me, but I would like someone else to review as well.

@levitte levitte requested a review from mattcaswell May 27, 2019 08:10
slontis
slontis reviewed May 27, 2019
View reviewed changes
slontis
slontis reviewed May 27, 2019
View reviewed changes
slontis
slontis reviewed May 27, 2019
View reviewed changes
slontis
slontis reviewed May 27, 2019
View reviewed changes
@slontis
Copy link
Member

slontis commented May 27, 2019

Looks good to me..

@paulidale
Copy link
Contributor Author

paulidale commented May 27, 2019

Update I hope.

@mspncp
Copy link
Contributor

mspncp commented May 28, 2019

@paulidale here is my review, as promised. It turned out to be more convenient for me to simply apply the changes to the pod file locally instead of using GitHub annotations.

see mspncp@b1c01bd

All changes are only suggestions. pick whatever you like. Just two final remarks:

  • I'd prefer if we would use lower case property names throughout. Which means in particular, the FIPS property should be named fips and not FIPS.
  • For a non-native speaker, the expression "tied results" might not be obvious. That's why I changed the formulation, not because I didn't like it. If you prefer your's, it's ok for me, too.

@paulidale
Copy link
Contributor Author

paulidale commented May 28, 2019

I like your version better.

@mspncp
Copy link
Contributor

mspncp commented May 28, 2019 •
edited
Loading

I like your version better.

Ok, thanks! If you like, I can fix the comments you made on my branch and then push my commit over to your pr branch? Or would you prefer pulling from my branch instead?

@mspncp
Copy link
Contributor

mspncp commented May 28, 2019

I just pushed my suggestions to your branch on your GitHub clone.

mspncp
mspncp reviewed May 28, 2019
View reviewed changes
mspncp
mspncp reviewed May 28, 2019
View reviewed changes
@paulidale
Copy link
Contributor Author

paulidale commented May 28, 2019

Just do a PR for your branch.

@mspncp
Copy link
Contributor

mspncp commented May 28, 2019

Just do a PR for your branch.

My changes are already on your branch on your clone. All you need is to pull them (or to reset your local branch).

@paulidale
Add man 7 page about properties.
b0b0ecf 
Add a page about properties in the man7 section of the public documentation.
@paulidale
Copy link
Contributor Author

paulidale commented May 28, 2019

Thanks, I've reformatted things so they fit on lines nicely and break lines at sentence boundaries.

levitte
levitte requested changes May 29, 2019
View reviewed changes
@mspncp
Copy link
Contributor

mspncp commented May 29, 2019

I'm not sure about which role the file crypto/property/properties.xhtml is intended to play. It does not seem to be linked to by any other part of the code. Is it intended to end up on some webpage or just meant to be lying around until someone discovers it and gets enchanted by the beauty of railroad diagrams? ;-)

mspncp
mspncp reviewed May 29, 2019
View reviewed changes
@mspncp
Copy link
Contributor

mspncp commented May 29, 2019

Is it intended to end up on some webpage or just meant to be lying around until someone discovers it and gets enchanted by the beauty of railroad diagrams? ;-)

At the least, you could mention the existence of the railroad diagram in the manual page following the grammar rules.

@paulidale
Copy link
Contributor Author

paulidale commented May 29, 2019

enchanted by the beauty of railroad diagrams? ;-)

You got it :)

mspncp
mspncp approved these changes May 29, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Approved, regardless of whether the "have and define" is changed or not.

levitte
levitte requested changes May 29, 2019
View reviewed changes
@paulidale
Copy link
Contributor Author

paulidale commented May 29, 2019

Comments addressed.

levitte
levitte approved these changes May 29, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mspncp
mspncp suggested changes May 29, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some documentation missing?

doc/man7/property.pod
Definition ::= PropertyName ( '=' Value )?
( ',' PropertyName ( '=' Value )? )*
Query ::= PropertyQuery ( ',' PropertyQuery )*
PropertyQuery ::= '-'? PropertyName
Copy link
Contributor

@mspncp mspncp May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the semantics of the optional '-' sign? I see that it's implemented in property_parse.c, but I can't find an explanation here in the documentation.

openssl/crypto/property/property_parse.c

Lines 394 to 400 in bb23d01

if (match_ch(&s, '-')) {
prop->oper = PROPERTY_OVERRIDE;
prop->optional = 0;
if (!parse_name(ctx, &s, 0, &prop->name_idx))
goto err;
goto skip_value;
}

Copy link
Contributor Author

@paulidale paulidale May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This opens a can of worms with two layers of property query and merging them together.

Copy link
Contributor Author

@paulidale paulidale May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this one and create a new PR to explain what's required.

levitte pushed a commit that referenced this pull request May 30, 2019
@paulidale
Add man 7 page about properties.
cec3901 
Add a page about properties in the man7 section of the public documentation.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #9011)
@paulidale
Copy link
Contributor Author

paulidale commented May 30, 2019

Merged, thanks for the reviews.

@paulidale paulidale closed this May 30, 2019
@paulidale paulidale mentioned this pull request May 30, 2019
Closed
1 task
@paulidale
Copy link
Contributor Author

paulidale commented May 30, 2019

Continued in #9042

@richsalz
Copy link
Contributor

richsalz commented May 30, 2019 

+Railroad diagrams for this grammar can be found in the
+F<crypto/property/properties.xhtml> file.

Is this the first time our docs have pointed to an explicit source file? Unfortunate.

@mspncp
Copy link
Contributor

mspncp commented May 30, 2019

Is this the first time our docs have pointed to an explicit source file? Unfortunate.

That came to my mind, too. My first thought was that the file needed to be installed together with the rest of the html documentation. But it occurred to me that the benefit would be little, unless the properties.xhtml file would be linked to properly from the html man page. And what about the regular manpages? That's when I stopped thinking about it, because I came to the conclusion the effort wasn't worth it. Anyway, the railroad diagram will probably only be interesting for developers, and those have source code available.

@paulidale
Copy link
Contributor Author

paulidale commented May 30, 2019

The reference is easy to remove if that is the consensus.

@mspncp
Copy link
Contributor

mspncp commented May 31, 2019

I abstain from voting :-)

@mattcaswell
Copy link
Member

mattcaswell commented May 31, 2019

I agree. We should not be referring to source files.

@mspncp
Copy link
Contributor

mspncp commented May 31, 2019

If that reference is to be removed, then consequently also the file crypto/property/properties.xhtml itself should be removed from the source code. Or at least moved elsewhere into the doc subdirectory. Having an unreferenced documentation file lying around between the source files is not very useful.

@paulidale
Copy link
Contributor Author

paulidale commented Jun 2, 2019

Reference removal in #9066

@paulidale paulidale deleted the property-doc branch June 2, 2019 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slontis slontis slontis left review comments

@levitte levitte levitte approved these changes

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

+1 more reviewer

@mspncp mspncp mspncp requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@paulidale @levitte @slontis @mspncp @richsalz @mattcaswell