As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review
addresses review comments from attachment 276229 (11.23 KB, patch)
2007-08-21 19:34 PDT, Neil Williams
nelson: review-
Details | Diff | Splinter Review
fixed bug with long form-only options (11.44 KB, patch)
2007-08-29 12:35 PDT, Neil Williams
nelson: review+
Details | Diff | Splinter Review
patch for testing patch 278826 (2.50 KB, patch)
2007-08-29 12:40 PDT, Neil Williams
no flags Details | Diff | Splinter 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 | Splinter Review

Description User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Neil Williams 2007-09-20 17:21:27 PDT
Created attachment 281758 [details] [diff] [review]
fix for 2 typos in SECU_ParseCommandLine
Comment 20 User image 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 User image 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.