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

[Niklas Söderlund]

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 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?

> 
> 	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

-- 
Regards,
Niklas Söderlund