390973 - Add long option names to SECU_ParseCommandLine

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Now that PL_CreateLongOptState is implemented in NSPR, SECU_ParseCommandLine,
the parser function used in most NSS utilities, needs to be modified to 
be able to use it.  

I thought there was already a bug for this and a patch that Alexei wrote, 
but I cannot find it.  It it exists, please add a comment to this bug, 
pointing to that bug.   Thanks.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Change SECU_ParseCommandLine to accept PLLongOpts

This patch adds a PLLongOpt pointer to the secuCommand struct used by 
SECU_ParseCommandLine,and changes SECU_ParseCommandLine to use it.

I will next attach a patch that demonstrates its use in cerutil.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: modify certutil to use long option names

This patch modifies certutil to have long option name equivalents to 
most of its existing single-character commands and options.  
This patch demonstrates adding long option names that are aliases for 
single-letter options, but does not demonstrate the addition of 
long option names that are NOT equivalent to single character names.
Maybe I should write another patch that demonstrates that. (?)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Commands I used to test the second patch:
certutil --Help
certutil --List
certutil --ListKeys
certutil --ListCerts "--nickname=DSA self signed"
certutil --Request --subject=CN=nelson,L=locality,ST=state,C=US \
         --keyname="DSA self signed" --ascii --phone [phone]
certutil --Request --subject=CN=nelson,L=locality,ST=state,C=US \
         --bits=512 --ascii --phone=9876543210
certutil --Modify --nickname="DSA self signed" --trust=",P,"
certutil --Modify --nickname "DSA self signed" --trust="P,,"

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: demonstrates use of long option not equivalent to single-character option

Here's a new options in certutil with no single-character equivalent.
To see how this was added, compare this patch with the previous one
in bugzilla.  Here's how it was tested.
> certutil --List --rutebega=pink
certutil: found rutebega option with value: pink

I'm not asking for review, because obviously I don't propose to commit this.

Comment 5

[Neil Williams]

Comment on attachment 275385 [details] [diff] [review]
Change SECU_ParseCommandLine to accept PLLongOpts

Nelson discussed how to merge the patch in bug 324744 with this one. I'll do that work and attach a new patch here.

Comment 6

[Neil Williams]

Attachment: Make necessary changes to SECU_ParseCommandLine for long options

This patch is a combination of attachment 275385 [details] [diff] [review] and one from bug 324744. This does not add long option names to certutil, bug 391719 has been opened to track that work.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 276229 [details] [diff] [review]
Make necessary changes to SECU_ParseCommandLine for long options

I emailed review comments to Neil.

Comment 8

[Neil Williams]

Attachment: addresses review comments from attachment 276229

In this version I've changed the type of flag back to char to avoid sign extension questions. Nelson, your question "indentation wierdness.  But worse, this will discard all single-character options that have no arguments.  (Right?)" is answered by this line from the plgetopt.h.

"When opt->option is zero, the token parsed was either a "long" (keyword) option or a positional parameter."

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 277649 [details] [diff] [review]
addresses review comments from attachment 276229 [details] [diff] [review]

r- for the reasons I emailed to Neil.

Comment 10

[Neil Williams]

Attachment: fixed bug with long form-only options

Nelson, in reply to your email you had 4 questions.

1) removed tabs.

2) I have checked all the NSS commands that use SECU_ParseCommandLine and all but certutil have static arrays of secuCommandFlag.

3) You're correct about the bug with "zero-flag secuCommandFlags". This patch addresses that.

4) Next patch changes certutil for testing purposes.

Comment 11

[Neil Williams]

Attachment: patch for testing patch 278826

This is the patch I used to test attachment #278826 [details] [diff] [review]. Here's the command line.

certutil -L -d "." --tokn tokename --extAIA

Not for review.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278826 [details] [diff] [review]
fixed bug with long form-only options

r=nelson

>     while ((status = PL_GetNextOpt(optstate)) == PL_OPT_OK) {
>+	const char *optstatelong;
>+	char        option = optstate->option;

>+            if (option == '\0')
>+		option = '\0377';       /* force unequal with all flags */

I agree that it's unlikely that anyone will ever use 0xff as an option.
I think I would have declared the new variable "option" as an int, 
and then would have set it to a value that simply CANNOT exist in a char,
such as (e.g.) 0x10000.  But I'm OK with this, and I know it avoids 
certain issues about signed vs unsigned chars and sign extension.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278827 [details] [diff] [review]
patch for testing patch 278826

This patch doesn't output anything that shows what was actually parsed.  I guess this is for testing in a debugger?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278826 [details] [diff] [review]
fixed bug with long form-only options

Neil, there is one problem with this patch that I caught before in a 
previous review, but not this time.  An extra comma has been added
after the last entry in a list of initializers.  On some platforms,
(including AIX) this will cause another "row" to be created. 
So the extraneous comma must come out.

The change was:
> 	{ /* opt_Hash                */  'Z', PR_TRUE,  0, PR_FALSE },
>-	{ /* opt_NewPasswordFile     */  '@', PR_TRUE,  0, PR_FALSE }
>+	{ /* opt_NewPasswordFile     */  '@', PR_TRUE,  0, PR_FALSE },
> };

Just undo that one change, removing the trailing comma.  Thanks.
If you've already committed this patch, then please commit another
patch just to fix this.

Comment 15

[Neil Williams]

Reply to comment #13: Yes I ran it under dbx.

Reply to comment #14: That comma went in when I diffed with version 1.116 of certutil. What I committed was based on 1.117 which didn't have the extra comma.

Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.118; previous revision: 1.117
done
Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.78; previous revision: 1.77
done
Checking in cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.22; previous revision: 1.21
done

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Assigning to Neil since he did the work.
This bug is P1 since it blocks two other P1 bugs.

Comment 17

[Neil Williams]

Attachment: sample program which reproduces bug

Bob Relyea ran into a couple of bugs in the latest patch. This attachment reproduces one of them. The patch that follows this fixes what he reported, I believe. Without the fix the cmd does:

x -B
parse failed 0

with it

$ x -B
cmd 'B'/"-" =

Comment 18

[Neil Williams]

Bob, could check out the test program and the fix? See if it fixes what you found?

Comment 19

[Neil Williams]

Attachment: fix for 2 typos in SECU_ParseCommandLine

Comment 20

[Robert Relyea]

Comment on attachment 281758 [details] [diff] [review]
fix for 2 typos in SECU_ParseCommandLine

r+ thanks for fixing this neil!

bob

Comment 21

[Neil Williams]

Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.80; previous revision: 1.79
done