[libcamera-devel] [PATCH 4/4] cam: Improve when usage information is printed

[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 at 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 at ideasonboard.com>
> > 
> >>  }
> >>  
> >>  static bool configureStreams(Camera *camera, std::vector<Stream *> &streams)

-- 
Regards,

Laurent Pinchart