Incorrect parsing of core parameters (issue #36)

[jwadolowski]

This is PR that fixes #36.

[jwadolowski]

The goal of this PR is to accept -p password, -u user and -i http://<url>:<port> but not -ppassword, -uuser and -ihttp://<url>:<port> (space between argument and value is required).

By default getopts accepts both ways, but to fix #36 we should decide on a single way.

getopts uses ${OPTIND} to identify arguments and it starts counting with 1. On the other hand sh enumerates all parameters starting from 0.

Let's use the following example:

$ ./cqapi -P -utest1 -u xxx -v '-u test3'

This is how arguments are enumerated by getopts and sh respectively:

#	OPTIND	Shell argument
0	-	./cqapi
1	./cqapi	-P
2	P	-utest1
3	test1	-u
4	u	xxx
5	xxx	-v
6	v	-u test3
7	test3	-

All in all OPTIND is always 1 element ahead of shell argument. Thanks to this relation we can compare these to values and determine if the arguments were bunched together or used separately.

The whole solution assumes that '' are always used when passing argument values instead of "". Motivation is quite simple - strings enclosed in single quotes are not evaluated and for example -v '<!--' won't be interpreted by shell (<, !, -, -- are reserved shell characters).

I can of course use double quotes and escape such values, but the problem is that I can't rely on this data when I'd like to compare values I set to information returned by CQ. Imagine I set key1 to <!-- using -s "key1" -v "\<\!--" during execution of cqcfg command. I will end up with a failure when I compare <!-- (returned by CQ) to "\<\!--" (cqcfg usage), as these strings are different.

The problem described above happened to me when I was implementing OSGi configuration management in Chef.

All in all I can see 2 solutions here - either this PR will be merged or I should get back to using "" and escape reserved shell characters before cqcfg command is executed but after values are defined in Chef recipe. Thanks to that so I can safely compare values defined in Chef (not escaped ones) to data returned by CQ (not escaped either).

[kitarek]

Hi Jakub,

thanks a lot. Unfortunately your patch works dash but not for bash. Please let me give a few days for that because definately the bug you have reported is critical and it should be fixed the proper way without need to use double quotes. It should work in any conditions.

Thanks, Arek

[jwadolowski]

I was wondering how we can fix the issue and some idea came to my mind - what if we can URL encode all parameters before getopts is executed? It should allow us to keep the original (and tested) logic when it comes to getopts.

[kitarek]

You mean only arguments but not options itself? The only one problem with that approach is that you can no longer use ${@} and you need to construct the whole params array by itself.

I will treat this as the highest priority issue right now. I will let you know when I find sth interesting that could resolve our problem.

[jwadolowski]

Thx Arek! I really do appreciate that.

You're right, above approach forces you to maintain a separate structure to iterate over, which is neither convenient nor easy to introduce.

Another option I can see is to add a parameter to cqcfg that skips auto URL encoding (cqapi -f), so all parameters will be passed on to curl as they are, w/o any preprocessing in between.

[kitarek]

I have eventually fixed the problem so options that looks like -uusername -ppassword and -iinstance aren't processed as options anymore. The fix should also work for very long command lines with many arguments.

Please note that if you try to pass as option any custom option argument: -u, in example:

cqcfg -u admin -s name -v '-u'

where -u is argument for custom option -v, this still won't be working correctly.

That's why I recommend to pass options -u, -i and -p at the end of command line to avoid such processing. The quick fix also is to allow remembering only the first occurrence of option but this will limit flexibility on command line.

The only one valid solution is to process command line only once by cqapi without need to process in custom commands which is not planned yet and it's major change for the whole toolkit.

Please let me know if it works for you correctly. The fix is in 1.2-dev branch.

Thanks, Arek

[jwadolowski]

Hi Arek,

I've successfully tested your changes in 1.2-dev branch. Indeed, options in combined format (-ppassword) are not accepted anymore. I can also confirm that crucial options (-u, -p and -i) should still be used at the end of command as you suggested.

I believe this closes this PR and related issue #36.

PS. You suggested on Skype that a more robust solution can be implemented (_getopts wrapper on top of regular getopts), so feel free to raise an improvement for 1.3+ releases to keep this on the roadmap.

Thanks again!

Cheers, Kuba