Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update s_client and s_server documentation about some missing arguments #1837

Closed
wants to merge 1 commit into from
Closed

Update s_client and s_server documentation about some missing arguments #1837

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2016

No description provided.

kaduk
kaduk reviewed Nov 3, 2016
View reviewed changes
doc/man1/s_server.pod

The TCP port to listen on for connections. If not specified 4433 is used.

=item B<-accept val>

The TCP optional ost and port to listen on for connections. If not specified 4433 is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ost/host/?
Is only the host optional so that the default is just a port? This text is not clear.

Copy link
Author

Choose a reason for hiding this comment

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

'host', yes. Sorry for the typo.

doc/man1/s_server.pod
@@ -125,14 +130,34 @@ manual page.

Print out a usage message.

=item B<-accept port>
=item B<-port port>
Copy link
Contributor

Choose a reason for hiding this comment

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

s_server's -port argument was present but undocumented (even in usage()) in SSLeay 0.8.1b, the earliest point in this git repo. One might wonder whether it was intentionally undocumented.

doc/man1/s_server.pod

=item B<-unlink>

For -unix, unlink existing socket first.
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of these could probably be reworded slightly better, for example, this one could be "unlink any existing socket before creating a new socket", but wordsmithing via pull-request comments is probably not worth the effort.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with all your comments. I have just taken the text from the -help option of each command. My english isn't good enought to supply better sentences.
:-(

richsalz
richsalz suggested changes Nov 4, 2016
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

some minor wording tweaks, hope this helps.

doc/man1/s_client.pod
@@ -132,6 +135,18 @@ When used with the B<-connect> flag, the program uses the host and port
specified with this flag and issues an HTTP CONNECT command to connect
to the desired server.

=item B<-unix val>

Connect over Unix domain sockets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "path" instead of val, and "Connect over the specified Unix-domain socket"

doc/man1/s_server.pod

The TCP port to listen on for connections. If not specified 4433 is used.

=item B<-accept val>

The TCP optional host and port to listen on for connections. If not specified 4433 is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

"The optional TCP host..." And "specified, "*:4433" is used" (Not quotes around the *:4433.

@richsalz richsalz added branch: master Merge to master branch 1.1.0 labels Nov 4, 2016
@ghost
Copy link
Author

ghost commented Nov 4, 2016

I am completely lost with Git / GitHub :-(
Looks like I added unrelated commit (SRP one) into this pull request, Sorry for that.

richsalz
richsalz approved these changes Nov 4, 2016
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

Looks good. EXCEPT that the statem_srvr change is NOT part of his PR.

@ghost
Copy link
Author

ghost commented Nov 4, 2016

Guess I should have created a distinct branch for each topic to avoid my last commit to join the 'current' PR ?

@richsalz
Copy link
Contributor

richsalz commented Nov 5, 2016

No, one merge request with all your changes in it is what we want. Do something like this to undo the wrong-file changes:

git diff master ssl/statem/statem_srvr.c | patch -R
# verify that your changes are "undone"
git add ssl/statem/statem_srvr.c 
git commit --am --no-e
git push -f github HEAD

@ghost
Copy link
Author

ghost commented Nov 5, 2016

Thanks Ritch for helping me with git. Seems that I was able to remove the last commit, but it was not without pain... (I am working under Windows using TortoiseGit).
Next big challenge : pushing the removed commit into its own, ANOTHER PR ...
;-)

levitte pushed a commit that referenced this pull request Nov 13, 2016
Update s_client and s_server documentation about some missing arguments
a22f9c8 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1837)
levitte pushed a commit that referenced this pull request Nov 13, 2016
Update s_client and s_server documentation about some missing arguments
d67fe76 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1837)
(cherry picked from commit a22f9c8)
@richsalz
Copy link
Contributor

a22f9c8 on master and d67fe76 on 1.1.0 thank you!

@richsalz richsalz closed this Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@richsalz @kaduk @mattcaswell