1321806 - Error out if version range cannot be parsed

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Daiki Ueno [:ueno]]

Attachment: proposed patch

When running selfserv/strsclnt/tstclnt with -V ssl2: or any other non-parsable version range, it prints the help message and continue running.  I think it would make more sense to abort when the tools see an invalid version range.

Comment 1

[Kai Engert (:KaiE:)]

I think the tools don't continue to run after the bada parameters are detected. You don't need to call exit(), because the function Usage() calls exit() after printing the usage message.

Many places in the parameter parsing already call Usage() to exit on failure, I think the call to PL_DestroyOptState(optstate) is unnecessary, since almost none of the existing places do it.

I agree that it's a good idea to print the additional message that explains which parameter exactly was incorrect.

I think we don't need to print the statement "-h for usage", because the usage is printed by default, when executed without commands, so it's trivial to discover the usage.

I suggest to simplify your patch, and only add the fprint("Bad...") statement prior to the existing call to Usage().

In other words, I suggest to replace
  Usage(progName);
with
  fprintf(stderr, "Bad version for parameter -V specified.\n");
  Usage(progName);

Or alternatively, if you don't like that the error message might potentially scroll out, because the Usage output is very long, you could also use:
  fprintf(stderr, "Bad version for parameter -V specified.\n");
  exit(1);

(It seems that tstclnt consistently uses exit code 1 for all scenarios that fail to parse the parameters.)

Comment 2

[Daiki Ueno [:ueno]]

Attachment: version-range-v2.patch

Thank you for the review.  I'm attaching an updated patch, which should address the issues.  One thing, however, is that selfserv.c:Usage doesn't call exit() and even has a '-h' option that calls exit(0) after Usage().  Perhaps it might be better to remove the option and always call exit() in Usage(), for consistency.

Comment 3

[Kai Engert (:KaiE:)]

Thank you for explaining the difference in selfserv.

Establishing complete consistency might require more work, to ensure it doesn't create new side effects.

The exit() statement that you added seems like a good minimal solution for now.

Comment 4

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/e2804da8af30