Closed Bug 390973 Opened 17 years ago Closed 17 years ago

Add long option names to SECU_ParseCommandLine

Categories

(NSS :: Tools, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: neil.williams)

References

Details

Attachments

(6 files, 3 obsolete files)

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.
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?
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)
Attachment #275385 - Flags: review? → review?(neil.williams)
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,,"
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.
Attachment #275385 - Flags: superreview?(alexei.volkov.bugs) → superreview+
Attachment #275387 - Flags: superreview?(alexei.volkov.bugs) → superreview+
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)
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)
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-
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)
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-
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)
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 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+
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 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.
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
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
Attachment #275387 - Flags: review?(neil.williams)
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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'/"-" =
Bob, could check out the test program and the fix? See if it fixes what you found?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #281758 - Flags: review?(rrelyea)
Attachment #281758 - Flags: review+
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+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: