Last Comment Bug 390973 - Add long option names to SECU_ParseCommandLine
: Add long option names to SECU_ParseCommandLine
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: trunk
: All All
: P1 enhancement (vote)
: 3.12
Assigned To: Neil Williams
:
Mentors:
Depends on: 346354
Blocks: 324740 324744
  Show dependency treegraph
 
Reported: 2007-08-04 19:18 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-09-24 12:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Change SECU_ParseCommandLine to accept PLLongOpts (5.09 KB, patch)
2007-08-05 23:05 PDT, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: superreview+
Details | Diff | Review
modify certutil to use long option names (8.13 KB, patch)
2007-08-05 23:08 PDT, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: superreview+
Details | Diff | Review
demonstrates use of long option not equivalent to single-character option (9.25 KB, patch)
2007-08-05 23:41 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
Make necessary changes to SECU_ParseCommandLine for long options (11.32 KB, patch)
2007-08-10 18:56 PDT, Neil Williams
nelson: review-
Details | Diff | Review
addresses review comments from attachment 276229 (11.23 KB, patch)
2007-08-21 19:34 PDT, Neil Williams
nelson: review-
Details | Diff | Review
fixed bug with long form-only options (11.44 KB, patch)
2007-08-29 12:35 PDT, Neil Williams
nelson: review+
Details | Diff | Review
patch for testing patch 278826 (2.50 KB, patch)
2007-08-29 12:40 PDT, Neil Williams
no flags Details | Diff | Review
sample program which reproduces bug (3.38 KB, text/x-csrc)
2007-09-20 17:17 PDT, Neil Williams
no flags Details
fix for 2 typos in SECU_ParseCommandLine (1.17 KB, patch)
2007-09-20 17:21 PDT, Neil Williams
rrelyea: review+
nelson: review+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2007-08-04 19:18:13 PDT
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) 2007-08-05 23:05:46 PDT
Created attachment 275385 [details] [diff] [review]
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) 2007-08-05 23:08:51 PDT
Created attachment 275387 [details] [diff] [review]
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) 2007-08-05 23:20:22 PDT
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 1234567890
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) 2007-08-05 23:41:24 PDT
Created attachment 275393 [details] [diff] [review]
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 2007-08-08 17:27:17 PDT
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 2007-08-10 18:56:39 PDT
Created attachment 276229 [details] [diff] [review]
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) 2007-08-14 20:28:55 PDT
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 2007-08-21 19:34:39 PDT
Created attachment 277649 [details] [diff] [review]
addresses review comments from attachment 276229 [details] [diff] [review]

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) 2007-08-23 21:49:27 PDT
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 2007-08-29 12:35:55 PDT
Created attachment 278826 [details] [diff] [review]
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 2007-08-29 12:40:23 PDT
Created attachment 278827 [details] [diff] [review]
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) 2007-08-29 13:16:59 PDT
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) 2007-08-29 13:19:56 PDT
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) 2007-08-29 15:23:25 PDT
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 2007-08-30 15:43:18 PDT
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) 2007-08-30 15:55:01 PDT
Assigning to Neil since he did the work.
This bug is P1 since it blocks two other P1 bugs.
Comment 17 Neil Williams 2007-09-20 17:17:07 PDT
Created attachment 281756 [details]
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 2007-09-20 17:19:00 PDT
Bob, could check out the test program and the fix? See if it fixes what you found?
Comment 19 Neil Williams 2007-09-20 17:21:27 PDT
Created attachment 281758 [details] [diff] [review]
fix for 2 typos in SECU_ParseCommandLine
Comment 20 Robert Relyea 2007-09-21 10:30:52 PDT
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 2007-09-21 12:51:40 PDT
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

Note You need to log in before you can comment on or make changes to this bug.