Add long option names to SECU_ParseCommandLine

RESOLVED FIXED in 3.12

Status

NSS
Tools
P1
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Neil Williams)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

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.
(Reporter)

Comment 1

10 years ago
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.
Attachment #275385 - Flags: superreview?(alexei.volkov.bugs)
Attachment #275385 - Flags: review?
(Reporter)

Comment 2

10 years ago
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. (?)
Attachment #275387 - Flags: superreview?(alexei.volkov.bugs)
Attachment #275387 - Flags: review?(neil.williams)
(Reporter)

Updated

10 years ago
Attachment #275385 - Flags: review? → review?(neil.williams)
(Reporter)

Comment 3

10 years ago
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,,"
(Reporter)

Comment 4

10 years ago
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.

Updated

10 years ago
Attachment #275385 - Flags: superreview?(alexei.volkov.bugs) → superreview+

Updated

10 years ago
Attachment #275387 - Flags: superreview?(alexei.volkov.bugs) → superreview+
(Assignee)

Comment 5

10 years ago
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.
Attachment #275385 - Attachment is obsolete: true
Attachment #275385 - Flags: review?(neil.williams)
(Assignee)

Comment 6

10 years ago
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.
Attachment #276229 - Flags: review?(nelson)
(Reporter)

Comment 7

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

I emailed review comments to Neil.
Attachment #276229 - Flags: review?(nelson) → review-
(Assignee)

Comment 8

10 years ago
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."
Attachment #276229 - Attachment is obsolete: true
Attachment #277649 - Flags: review?(nelson)
(Reporter)

Comment 9

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

r- for the reasons I emailed to Neil.
Attachment #277649 - Flags: review?(nelson) → review-
(Assignee)

Comment 10

10 years ago
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.
Attachment #277649 - Attachment is obsolete: true
Attachment #278826 - Flags: review?(nelson)
(Assignee)

Comment 11

10 years ago
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.
(Reporter)

Comment 12

10 years ago
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.
Attachment #278826 - Flags: review?(nelson) → review+
(Reporter)

Comment 13

10 years ago
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?
(Reporter)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
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
Status: NEW → ASSIGNED
(Reporter)

Comment 16

10 years ago
Assigning to Neil since he did the work.
This bug is P1 since it blocks two other P1 bugs.
Assignee: nelson → neil.williams
Status: ASSIGNED → NEW
Priority: -- → P1
(Reporter)

Updated

10 years ago
Attachment #275387 - Flags: review?(neil.williams)
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

10 years ago
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'/"-" =
(Assignee)

Comment 18

10 years ago
Bob, could check out the test program and the fix? See if it fixes what you found?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

10 years ago
Created attachment 281758 [details] [diff] [review]
fix for 2 typos in SECU_ParseCommandLine
Attachment #281758 - Flags: review?(rrelyea)
(Reporter)

Updated

10 years ago
Attachment #281758 - Flags: review+

Comment 20

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

r+ thanks for fixing this neil!

bob
Attachment #281758 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 21

10 years ago
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
Status: REOPENED → ASSIGNED
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.