ZOOKEEPER-3599: cli.c: Resuscitate "old-style" argument parsing #1131

ztzg · 2019-10-31T09:48:02Z

ZOOKEEPER-2122 added SSL support to the C client and to the cli_st/mt tools. It introduced (much-welcome!) GNU-style getopt_long argument parsing, but did not try to preserve backwards compatibility.

An earlier version of this #1131 introduced (optional) POSIX-style getopt argument parsing, and was consequently largely obsoleted by the SSL update. It did, however, support "old-style" arguments to avoid breaking workflows.

This patch salvages that feature while preserving the getopt_long goodness. It is "legacy-only"; adding new positional arguments is not supported—as discussed in this thread: #1131 (review).

ztzg · 2019-11-21T12:47:21Z

This PR became partially obsolete with the merge of ZOOKEEPER-2122 into master ( #1107).

The latter makes getopt_long (as opposed to getopt) a hard requirement, and includes a header-only implementation for Windows platforms. Fair enough, I suppose.

A big difference, however, is that resulting argument parsing logic is not backwards-compatible: running cli_st <connect-string> exits with status 2 after printing an usage banner, whereas it was fine before. Same for cmd:, etc.

@eolivelli, @anmolnar, @symat, @suhasdantkale: What do you think: should I just drop this PR and the associated ticket, or should I try and "salvage" the backwards compatibility bits? (I, for one, won't be sad to see old-style argument parsing disappear, but it might break some workflows and/or docs.)

eolivelli

I sponsor this idea of enhancing the C cli.

I don't know how many users of the C client exist, I think we are not suggesting users to use this cli. So changing the behavior is not an hard breaking change.

Personally I have always thought about the C cli as an example of how to use the C client.

I am not a C expert so I will leave a good code review to the other committers.

+0 from me

Thank you for doing this

symat · 2019-11-21T14:53:47Z

Sorry, I didn't see this PR before.

I actually much prefer the getopt way, as it provides more standard unix-like and extendable argument handling. Using the header-only implementation for windows keeps the code easier to understand and maintain (less ifdef in the code). Also I think it is a good idea if the user can use the same CLI syntax on all platforms.

In general I prefer backward compatible changes. In the case of the C command line client, I am not that sure. I don't think people would use it heavily, especially not as an interface. But maybe you are right and someone is using / calling it from some devops or monitoring tool. In this case it would be nice to keep the old argument parsing.

If you have the time to work on this and make command line arguments backward compatible, then I would suggest to add it back only as a secondary fallback option. Like if the new getopt argument parsing fails (e.g. because the new mandatory server url parameter is missing) then we can try to parse the command line in the old way. But I wouldn't extend the old command line arguments anymore (e.g. I wouldn't add the SSL or debug options for the old command line parsing). I would also add some comments to the code, to explain why we accept two syntaxes.

anmolnar · 2019-11-21T16:14:29Z

@ztzg The other patch #1107 has been submitted now. Please rebase this one.
I think you're free to change the argument parsing of the CLI. It would be nice if we could keep backward compatibility at all times, so I think it worth a try. Otherwise please indicate in the docs clearly that a breaking change is introduced in this version.

ztzg · 2019-11-21T16:42:52Z

@eolivelli, @symat, @anmolnar:

I think we all agree that getopt-only is the way to go for new flags. I thought I would just close this when I saw the change landing via #1107, but started wondering about compatibility—particularly as I already had a working "fix" in this PR.

If you have the time to work on this and make command line arguments backward compatible, then I would suggest to add it back only as a secondary fallback option.

Secondary/fallback parsing sounds reasonable. I'll look into it as I have to rebase related work on top of #1107 anyway.

There is no reason for it not to be--except for a silly const- correctness issue in 'strtol', and we can work around the latter by introducing a temporary variable.

The pointed-to string has the same lifetime as the argv[] entries, as per POSIX: https://pubs.opengroup.org/onlinepubs/009695399/functions/getopt.html https://stackoverflow.com/a/53508112 This avoids a tiny memory leak for options passed more than once :)

ZOOKEEPER-2122 added SSL support to the C client and to the cli_st/mt tools. It introduced a (much-welcome!) GNU-style 'getopt_long' argument parsing, but did not try to preserve backwards compatibility. An earlier version of apache#1131 introduced optional POSIX-style 'getopt' argument parsing, and was consequently largely obsoleted by SSL update. It did, however, support "old-style" argument parsing, to avoid breaking workflows. This patch salvages that feature, while preserving the 'getopt_long' goodness. It is "legacy-only"; adding new positional arguments is not supported--as discussed in this thread: apache#1131 (review)

ztzg · 2019-11-27T08:21:07Z

The latest PR failed to build because one of the Jenkins workers was did not have the "standard" OpenSSL configuration: #1107 (comment); This should be fixed by ZOOKEEPER-3630.

ztzg · 2019-12-04T08:14:03Z

retest maven build

asf-ci · 2019-12-04T08:54:24Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1684/

Build result: FAILURE

[...truncated 928.35 KB...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/pom.xml to org.apache.zookeeper/parent/3.6.0-SNAPSHOT/parent-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-lock/pom.xml to org.apache.zookeeper/zookeeper-recipes-lock/3.6.0-SNAPSHOT/zookeeper-recipes-lock-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/pom.xml to org.apache.zookeeper/zookeeper-contrib/3.6.0-SNAPSHOT/zookeeper-contrib-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/pom.xml to org.apache.zookeeper/zookeeper-client/3.6.0-SNAPSHOT/zookeeper-client-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/pom.xml to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-javadoc.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-queue/pom.xml to org.apache.zookeeper/zookeeper-recipes-queue/3.6.0-SNAPSHOT/zookeeper-recipes-queue-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/pom.xml to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/pom.xml to org.apache.zookeeper/zookeeper-recipes/3.6.0-SNAPSHOT/zookeeper-recipes-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/pom.xml to org.apache.zookeeper/zookeeper-client-c/3.6.0-SNAPSHOT/zookeeper-client-c-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-rest/pom.xml to org.apache.zookeeper/zookeeper-contrib-rest/3.6.0-SNAPSHOT/zookeeper-contrib-rest-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-assembly/pom.xml to org.apache.zookeeper/zookeeper-assembly/3.6.0-SNAPSHOT/zookeeper-assembly-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-zooinspector/pom.xml to org.apache.zookeeper/zookeeper-contrib-zooinspector/3.6.0-SNAPSHOT/zookeeper-contrib-zooinspector-3.6.0-SNAPSHOT.pomchannel stopped[SpotBugs] Skipping execution of recorder since overall result is 'FAILURE'Setting status of b4e3106 to FAILURE with url https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1684/ and message: 'FAILURE 'Using context: JenkinsMaven

ztzg · 2019-12-04T09:12:18Z

retest maven build

asf-ci · 2019-12-04T09:59:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1685/

symat · 2019-12-04T12:58:13Z

looks great, thank you!

I did some testing with your patch and I am not sure if it is caused by this PR or not, but batch mode seems not working now with cli_mt, only working with cli_st:

using cli_st with the new and old arguments:

root@8697285cbb47:/git/zookeeper/zookeeper-client/zookeeper-client-c/target/c/bin# ./cli_st --host localhost:2181 --cmd 'ls /'
Batch mode: ls /
Watcher SESSION_EVENT state = CONNECTED_STATE
Got a new session id: 0x1000e71b8340014
time = 4 msec
/: rc = 0
	zookeeper
time = 4 msec


root@8697285cbb47:/git/zookeeper/zookeeper-client/zookeeper-client-c/target/c/bin# ./cli_st localhost:2181 cmd:'ls /'
Batch mode: 'ls /'
Watcher SESSION_EVENT state = CONNECTED_STATE
Got a new session id: 0x1000e71b8340015
time = 6 msec
/: rc = 0
	zookeeper
time = 6 msec


root@8697285cbb47:/git/zookeeper/zookeeper-client/zookeeper-client-c/target/c/bin#

But doing the same with cli_mt seems to be broken for me:

root@8697285cbb47:/git/zookeeper/zookeeper-client/zookeeper-client-c/target/c/bin# ./cli_mt --host localhost:2181 --cmd 'ls /'
Batch mode: ls /
Watcher SESSION_EVENT state = CONNECTED_STATE
Got a new session id: 0x1000e71b8340016
< waits forever here.. >
^C

root@8697285cbb47:/git/zookeeper/zookeeper-client/zookeeper-client-c/target/c/bin# ./cli_mt localhost:2181 cmd:'ls /'
Batch mode: 'ls /'
Watcher SESSION_EVENT state = CONNECTED_STATE
Got a new session id: 0x1000e71b8340017
< waits forever here.. >
^C

I was building using mvn clean install -Pfull-build -DskipTests on top of an ubuntu 18.4 docker image, with the following extra packages installed: apt update && apt install -y libcppunit-dev maven default-jdk-headless autoconf libtool g++ make software-properties-common pkg-config wget git python-setuptools openssl libssl-dev

ztzg · 2019-12-09T08:24:09Z

Hi @symat,

Thank you for the test, and report. Nice catch! That was a bit puzzling... but it turns out that "batch mode" has never been implemented in cli_mt.

From the first day on, cmd was only handled in the #else /* !THREADED */ section.

I have now opened ZOOKEEPER-3640, "Implement "batch mode" in cli_mt".

If you are otherwise happy with the PR, could we get this moving? I'd like to get the pipeline flushed and get a rebased/aligned SASL client sitting on top :)

symat · 2019-12-09T08:34:48Z

yep, thanks for clarifying :)
otherwise it looks good to me. (but I am not a committer to officially +1 it)

@nkalmar @anmolnar FYI

ztzg · 2019-12-09T08:51:09Z

Okay—Thanks!

nkalmar

I'm not much competent in C either, but since it's a cli change to preserve backward compatibility, it looks safe to me. I'll leave the patch open for now, see if any further comment comes until, let's say, tomorrow. Or any other committers are free to merge earlier. Or if @eolivelli changes he's +0 to +1 I'll merge :)

symat

LGTM

nkalmar · 2019-12-11T15:06:58Z

Merged to master. Thanks @ztzg

ZOOKEEPER-2122 added SSL support to the C client and to the `cli_st`/`mt` tools. It introduced (much-welcome!) GNU-style `getopt_long` argument parsing, but did not try to preserve backwards compatibility. An earlier version of this apache#1131 introduced (optional) POSIX-style `getopt` argument parsing, and was consequently largely obsoleted by the SSL update. It did, however, support "old-style" arguments to avoid [breaking workflows](https://xkcd.com/1172/). This patch salvages that feature while preserving the `getopt_long` goodness. It is "legacy-only"; adding new positional arguments is not supported—as discussed in this thread: apache#1131 (review). Author: Damien Diederen <[email protected]> Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]> Closes apache#1131 from ztzg/ZOOKEEPER-3599-use-getopt-in-cli-c

eolivelli approved these changes Nov 21, 2019

View reviewed changes

ztzg added 3 commits November 21, 2019 18:41

ZOOKEEPER-3599: cli.c: Make processline's argument const

ea1f713

There is no reason for it not to be--except for a silly const- correctness issue in 'strtol', and we can work around the latter by introducing a temporary variable.

ztzg force-pushed the ZOOKEEPER-3599-use-getopt-in-cli-c branch from 255bc26 to b4e3106 Compare November 21, 2019 18:19

ztzg changed the title ~~ZOOKEEPER-3599: cli.c: Introduce (optional) 'getopt' option parsing~~ ZOOKEEPER-3599: cli.c: Resuscitate "old-style" argument parsing Nov 21, 2019

nkalmar approved these changes Dec 10, 2019

View reviewed changes

symat approved these changes Dec 11, 2019

View reviewed changes

asfgit closed this in 1f33a71 Dec 11, 2019

ZOOKEEPER-3599: cli.c: Resuscitate "old-style" argument parsing #1131

ZOOKEEPER-3599: cli.c: Resuscitate "old-style" argument parsing #1131

Uh oh!

Conversation

ztzg commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ztzg commented Nov 21, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

symat commented Nov 21, 2019

Uh oh!

anmolnar commented Nov 21, 2019

Uh oh!

ztzg commented Nov 21, 2019

Uh oh!

ztzg commented Nov 27, 2019

Uh oh!

ztzg commented Dec 4, 2019

Uh oh!

asf-ci commented Dec 4, 2019

Build result: FAILURE

Uh oh!

ztzg commented Dec 4, 2019

Uh oh!

asf-ci commented Dec 4, 2019

Uh oh!

symat commented Dec 4, 2019

Uh oh!

ztzg commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symat commented Dec 9, 2019

Uh oh!

ztzg commented Dec 9, 2019

Uh oh!

nkalmar left a comment

Choose a reason for hiding this comment

Uh oh!

symat left a comment

Choose a reason for hiding this comment

Uh oh!

nkalmar commented Dec 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ztzg commented Oct 31, 2019 •

edited

Loading

ztzg commented Dec 9, 2019 •

edited

Loading