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

I like this and will make it so.

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