| Message ID | 20190220143736.529-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Feb 20, 2019 at 03:37:36PM +0100, Niklas Söderlund wrote: > Running the cam tool without any options results in the tool to exit > with EXIT_FAILURE but no usage being printed, this is confusing. Improve > this by also printing the usage text. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 522d2f0d3373dc25..9f4c8e26751d982c 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -41,6 +41,8 @@ void signalHandler(int signal) > > static int parseOptions(int argc, char *argv[]) > { > + int ret = 0; > + > KeyValueParser formatKeyValue; > formatKeyValue.addOption("width", OptionInteger, "Width in pixels", > ArgumentRequired); > @@ -67,15 +69,14 @@ static int parseOptions(int argc, char *argv[]) > parser.addOption(OptList, OptionNone, "List all cameras", "list"); > > options = parser.parse(argc, argv); > + > if (!options.valid()) > - return -EINVAL; > + ret = -EINVAL; > > - if (argc == 1 || options.isSet(OptHelp)) { Good catch, when no options are specified options.valid() returns false, I had overlooked that. > + if (ret || options.isSet(OptHelp)) > parser.usage(); > - return 1; > - } > > - return 0; > + return ret; How about simplifying this to options = parser.parse(argc, argv); if (!options.valid() || options.isSet(OptHelp)) { parser.usage(); return 1; } return 0; With this change, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > static bool configureStreams(Camera *camera, std::vector<Stream *> &streams)
Hi Laurent, Thanks for your feedback. On 2019-02-22 02:39:04 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Wed, Feb 20, 2019 at 03:37:36PM +0100, Niklas Söderlund wrote: > > Running the cam tool without any options results in the tool to exit > > with EXIT_FAILURE but no usage being printed, this is confusing. Improve > > this by also printing the usage text. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index 522d2f0d3373dc25..9f4c8e26751d982c 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -41,6 +41,8 @@ void signalHandler(int signal) > > > > static int parseOptions(int argc, char *argv[]) > > { > > + int ret = 0; > > + > > KeyValueParser formatKeyValue; > > formatKeyValue.addOption("width", OptionInteger, "Width in pixels", > > ArgumentRequired); > > @@ -67,15 +69,14 @@ static int parseOptions(int argc, char *argv[]) > > parser.addOption(OptList, OptionNone, "List all cameras", "list"); > > > > options = parser.parse(argc, argv); > > + > > if (!options.valid()) > > - return -EINVAL; > > + ret = -EINVAL; > > > > - if (argc == 1 || options.isSet(OptHelp)) { > > Good catch, when no options are specified options.valid() returns false, > I had overlooked that. > > > + if (ret || options.isSet(OptHelp)) > > parser.usage(); > > - return 1; > > - } > > > > - return 0; > > + return ret; > > How about simplifying this to > > options = parser.parse(argc, argv); > if (!options.valid() || options.isSet(OptHelp)) { > parser.usage(); > return 1; > } I considered this when creating the patch and decided I did not like the end result. I like the distinction that if something goes wrong a error code (which current design needs to be negative) is returned. With this change 'cam --help' would result in the return value from the tool would be EXIT_FAILURE. If we all are OK with this behavior for --help I would be open to take your suggestion and make parseOptions() return a bool instead of an int. What do you all think? > > return 0; > > With this change, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > } > > > > static bool configureStreams(Camera *camera, std::vector<Stream *> &streams) > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Fri, Feb 22, 2019 at 02:38:36AM +0100, Niklas Söderlund wrote: > On 2019-02-22 02:39:04 +0200, Laurent Pinchart wrote: > > On Wed, Feb 20, 2019 at 03:37:36PM +0100, Niklas Söderlund wrote: > >> Running the cam tool without any options results in the tool to exit > >> with EXIT_FAILURE but no usage being printed, this is confusing. Improve > >> this by also printing the usage text. > >> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> src/cam/main.cpp | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >> index 522d2f0d3373dc25..9f4c8e26751d982c 100644 > >> --- a/src/cam/main.cpp > >> +++ b/src/cam/main.cpp > >> @@ -41,6 +41,8 @@ void signalHandler(int signal) > >> > >> static int parseOptions(int argc, char *argv[]) > >> { > >> + int ret = 0; > >> + > >> KeyValueParser formatKeyValue; > >> formatKeyValue.addOption("width", OptionInteger, "Width in pixels", > >> ArgumentRequired); > >> @@ -67,15 +69,14 @@ static int parseOptions(int argc, char *argv[]) > >> parser.addOption(OptList, OptionNone, "List all cameras", "list"); > >> > >> options = parser.parse(argc, argv); > >> + > >> if (!options.valid()) > >> - return -EINVAL; > >> + ret = -EINVAL; > >> > >> - if (argc == 1 || options.isSet(OptHelp)) { > > > > Good catch, when no options are specified options.valid() returns false, > > I had overlooked that. > > > >> + if (ret || options.isSet(OptHelp)) > >> parser.usage(); > >> - return 1; > >> - } > >> > >> - return 0; > >> + return ret; > > > > How about simplifying this to > > > > options = parser.parse(argc, argv); > > if (!options.valid() || options.isSet(OptHelp)) { > > parser.usage(); > > return 1; > > } > > I considered this when creating the patch and decided I did not like the > end result. I like the distinction that if something goes wrong a error > code (which current design needs to be negative) is returned. > > With this change 'cam --help' would result in the return value from the > tool would be EXIT_FAILURE. If we all are OK with this behavior for > --help I would be open to take your suggestion and make parseOptions() > return a bool instead of an int. What do you all think? The help option usually results in no other option being processed and the application returning immediately. I think we should preserve that behaviour. We may not want to return EXIT_FAILURE in that case though. options = parser.parse(argc, argv); if (!options.valid() || options.isSet(OptHelp)) { parser.usage(); return !options.valid() ? -EINVAL : -EINTR; } And in the caller return ret == -EINTR ? 0 : EXIT_FAILURE; ? > > > > return 0; > > > > With this change, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> } > >> > >> static bool configureStreams(Camera *camera, std::vector<Stream *> &streams)
Hi Laurent, Thanks for your feedback. On 2019-02-22 11:57:34 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Fri, Feb 22, 2019 at 02:38:36AM +0100, Niklas Söderlund wrote: > > On 2019-02-22 02:39:04 +0200, Laurent Pinchart wrote: > > > On Wed, Feb 20, 2019 at 03:37:36PM +0100, Niklas Söderlund wrote: > > >> Running the cam tool without any options results in the tool to exit > > >> with EXIT_FAILURE but no usage being printed, this is confusing. Improve > > >> this by also printing the usage text. > > >> > > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > >> --- > > >> src/cam/main.cpp | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > >> index 522d2f0d3373dc25..9f4c8e26751d982c 100644 > > >> --- a/src/cam/main.cpp > > >> +++ b/src/cam/main.cpp > > >> @@ -41,6 +41,8 @@ void signalHandler(int signal) > > >> > > >> static int parseOptions(int argc, char *argv[]) > > >> { > > >> + int ret = 0; > > >> + > > >> KeyValueParser formatKeyValue; > > >> formatKeyValue.addOption("width", OptionInteger, "Width in pixels", > > >> ArgumentRequired); > > >> @@ -67,15 +69,14 @@ static int parseOptions(int argc, char *argv[]) > > >> parser.addOption(OptList, OptionNone, "List all cameras", "list"); > > >> > > >> options = parser.parse(argc, argv); > > >> + > > >> if (!options.valid()) > > >> - return -EINVAL; > > >> + ret = -EINVAL; > > >> > > >> - if (argc == 1 || options.isSet(OptHelp)) { > > > > > > Good catch, when no options are specified options.valid() returns false, > > > I had overlooked that. > > > > > >> + if (ret || options.isSet(OptHelp)) > > >> parser.usage(); > > >> - return 1; > > >> - } > > >> > > >> - return 0; > > >> + return ret; > > > > > > How about simplifying this to > > > > > > options = parser.parse(argc, argv); > > > if (!options.valid() || options.isSet(OptHelp)) { > > > parser.usage(); > > > return 1; > > > } > > > > I considered this when creating the patch and decided I did not like the > > end result. I like the distinction that if something goes wrong a error > > code (which current design needs to be negative) is returned. > > > > With this change 'cam --help' would result in the return value from the > > tool would be EXIT_FAILURE. If we all are OK with this behavior for > > --help I would be open to take your suggestion and make parseOptions() > > return a bool instead of an int. What do you all think? > > The help option usually results in no other option being processed and > the application returning immediately. I think we should preserve that > behaviour. We may not want to return EXIT_FAILURE in that case though. > > options = parser.parse(argc, argv); > if (!options.valid() || options.isSet(OptHelp)) { > parser.usage(); > return !options.valid() ? -EINVAL : -EINTR; > } > > And in the caller > > return ret == -EINTR ? 0 : EXIT_FAILURE; > > ? I like this and will make it so. > > > > > > > return 0; > > > > > > With this change, > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > >> } > > >> > > >> static bool configureStreams(Camera *camera, std::vector<Stream *> &streams) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index 522d2f0d3373dc25..9f4c8e26751d982c 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -41,6 +41,8 @@ void signalHandler(int signal) static int parseOptions(int argc, char *argv[]) { + int ret = 0; + KeyValueParser formatKeyValue; formatKeyValue.addOption("width", OptionInteger, "Width in pixels", ArgumentRequired); @@ -67,15 +69,14 @@ static int parseOptions(int argc, char *argv[]) parser.addOption(OptList, OptionNone, "List all cameras", "list"); options = parser.parse(argc, argv); + if (!options.valid()) - return -EINVAL; + ret = -EINVAL; - if (argc == 1 || options.isSet(OptHelp)) { + if (ret || options.isSet(OptHelp)) parser.usage(); - return 1; - } - return 0; + return ret; } static bool configureStreams(Camera *camera, std::vector<Stream *> &streams)
Running the cam tool without any options results in the tool to exit with EXIT_FAILURE but no usage being printed, this is confusing. Improve this by also printing the usage text. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)