Closed
Bug 390973
Opened 17 years ago
Closed 17 years ago
Add long option names to SECU_ParseCommandLine
Categories
(NSS :: Tools, enhancement, P1)
NSS
Tools
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: nelson, Assigned: neil.williams)
References
Details
Attachments
(6 files, 3 obsolete files)
8.13 KB,
patch
|
alvolkov.bgs
:
superreview+
|
Details | Diff | Splinter Review |
9.25 KB,
patch
|
Details | Diff | Splinter Review | |
11.44 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
Details | Diff | Splinter Review | |
3.38 KB,
text/x-csrc
|
Details | |
1.17 KB,
patch
|
rrelyea
:
review+
nelson
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Attachment #275385 -
Flags: review? → review?(neil.williams)
Reporter | ||
Comment 3•17 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•17 years ago
|
||
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•17 years ago
|
Attachment #275385 -
Flags: superreview?(alexei.volkov.bugs) → superreview+
Updated•17 years ago
|
Attachment #275387 -
Flags: superreview?(alexei.volkov.bugs) → superreview+
Assignee | ||
Comment 5•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
Attachment #275387 -
Flags: review?(neil.williams)
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•17 years ago
|
||
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•17 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•17 years ago
|
||
Attachment #281758 -
Flags: review?(rrelyea)
Reporter | ||
Updated•17 years ago
|
Attachment #281758 -
Flags: review+
Comment 20•17 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•17 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•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•