Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optparse: fix incorrect error message when invalid short option is grouped with valid short options #1183

Merged
merged 2 commits into from
Sep 9, 2017

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Sep 7, 2017

As described in #1179, optparse error message for unrecognized option is not correct when an invalid short option is used and grouped with one or more valid options.

Fix this by assuming an invalid short option when optopt != '\0' and use the char in optopt as the unrecognized option in the error output. Otherwise, assume a long option and use argv [optind-1] as the invalid option to display.

Simple tests were also added to ensure valid error messages for unrecognized short options, grouped and ungrouped.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 78.203% when pulling 71a46ad on grondo:issue#1179 into 9c860d0 on flux-framework:master.

@garlick
Copy link
Member

garlick commented Sep 8, 2017

This error is popping up in travis, in unrelated code. Not sure what to make of it.

make[4]: Entering directory `/home/travis/build/flux-framework/flux-core/flux-core-0.8.0-88-gb9d062d2/_build/sub/src/common/libsubprocess'
  CC       zio.lo
  CC       subprocess.lo
../../../../../src/common/libsubprocess/subprocess.c:1092:19: error: passing an
      object that undergoes default argument promotion to 'va_start' has
      undefined behavior [-Werror,-Wvarargs]
    va_start (ap, item);
                  ^
../../../../../src/common/libsubprocess/subprocess.c:1085:66: note: parameter of
      type 'sm_item_t' (aka 'enum sm_item') is declared here
subprocess_manager_set (struct subprocess_manager *sm, sm_item_t item, ...)

@grondo
Copy link
Contributor Author

grondo commented Sep 8, 2017

they did just update the trusty images recently, so perhaps a new warning in a small compiler update?

Actually this is a real error, apparently passing any value with a default type promotion before the ellipsis in a variadic function is a no-no. E.g. in this case the enum is promoted to an integer type and I guess isn't exactly defined what the size of the type would be.

Will have to find a quick fix.

https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start

@codecov-io
Copy link

codecov-io commented Sep 8, 2017

Codecov Report

Merging #1183 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1183      +/-   ##
==========================================
- Coverage   77.82%   77.78%   -0.04%     
==========================================
  Files         158      158              
  Lines       29255    29257       +2     
==========================================
- Hits        22768    22758      -10     
- Misses       6487     6499      +12
Impacted Files Coverage Δ
src/common/liboptparse/optparse.c 88.48% <100%> (+0.03%) ⬆️
src/common/libflux/mrpc.c 85.15% <0%> (-1.18%) ⬇️
src/common/libkvs/kvs_watch.c 86.34% <0%> (-0.89%) ⬇️
src/common/libflux/future.c 88.61% <0%> (-0.5%) ⬇️
src/common/libflux/message.c 80.94% <0%> (-0.36%) ⬇️
src/broker/overlay.c 71.67% <0%> (-0.35%) ⬇️
src/broker/module.c 83.28% <0%> (-0.28%) ⬇️
src/modules/kvs/kvs.c 63.16% <0%> (-0.27%) ⬇️
src/common/libkvs/kvs.c 65.08% <0%> (-0.26%) ⬇️
src/common/libflux/response.c 84.55% <0%> (ø) ⬆️
... and 4 more

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 78.192% when pulling badc987 on grondo:issue#1179 into 9c860d0 on flux-framework:master.

Emit the correct unrecognized option character on error when
short options are used and grouped on the commandline.

Fixes flux-framework#1179.
Add test that the correct unrecognized short option is displayed
in the optparse error messages when getopt calls out unknown option.
@grondo grondo changed the title optparse: fix invalid error message when invaled short option is grouped with valid short options optparse: fix incorrect error message when invalid short option is grouped with valid short options Sep 8, 2017
@grondo
Copy link
Contributor Author

grondo commented Sep 8, 2017

Rebased on current master, so we should make it through travis this time.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 78.135% when pulling aed7f57 on grondo:issue#1179 into ea1f316 on flux-framework:master.

@grondo
Copy link
Contributor Author

grondo commented Sep 8, 2017

One build hit a variant of #1169. Restarting.

@garlick
Copy link
Member

garlick commented Sep 9, 2017

Thanks!

@garlick garlick merged commit 8443eff into flux-framework:master Sep 9, 2017
@grondo grondo deleted the issue#1179 branch October 25, 2017 02:19
@grondo grondo mentioned this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants