[libcamera-devel,5/6] cam: options: add a key=value parser

426 diff mbox series

Some options passed to the cam utility needs to be complex and specify a
list of key=value pairs, add a new parser to deal white these options.
The new parser is integrated into the existing OptionsParser and the cam
application can fully describe all its options in one location and
perform full parsing of all arguments in one go. The new key=value
parsers also integrates itself with the usage() printing of the
OptionsParser.

The new parser can also be used on it's own to parse key=value pairs
from different data sources then the command line options.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/options.cpp | 159 +++++++++++++++++++++++++++++++++++++++++++-
 src/cam/options.h   |  35 ++++++++++
 2 files changed, 193 insertions(+), 1 deletion(-)

Hi Niklas,

Thank you for the patch.

On Mon, Jan 28, 2019 at 01:41:08AM +0100, Niklas Söderlund wrote:
> Some options passed to the cam utility needs to be complex and specify a

s/needs/need/

> list of key=value pairs, add a new parser to deal white these options.

s/, add/. Add/
s/white/with/

> The new parser is integrated into the existing OptionsParser and the cam
> application can fully describe all its options in one location and
> perform full parsing of all arguments in one go. The new key=value
> parsers also integrates itself with the usage() printing of the
> OptionsParser.
> 
> The new parser can also be used on it's own to parse key=value pairs

s/it's/its/

> from different data sources then the command line options.

s/then/than/

I would have split this patch in two, with one patch to add the
key-value parser, and another patch to integrate it in the options
parser.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/options.cpp | 159 +++++++++++++++++++++++++++++++++++++++++++-
>  src/cam/options.h   |  35 ++++++++++
>  2 files changed, 193 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index d3bff1cd897a5cfb..f63fd3b495e29986 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -30,11 +30,22 @@ bool OptionsParser::addOption(int opt, const char *help, const char *name,
>  	if (optionsMap_.find(opt) != optionsMap_.end())
>  		return false;
>  
> -	optionsMap_[opt] = Option({ opt, name, argument, argumentName, help });
> +	optionsMap_[opt] = Option({ opt, name, argument, argumentName, help,
> +				    nullptr });
>  
>  	return true;
>  }
>  
> +void OptionsParser::addOption(int opt, const char *help, KeyValueParser &parser,
> +			      const char *name)
> +{
> +	if (!addOption(opt, help, name, ArgumentRequired,
> +		       "key=value[,key=value,...]"))
> +		return;
> +
> +	optionsMap_[opt].keyValueParser = &parser;
> +}
> +
>  OptionsParser::Options OptionsParser::parse(int argc, char **argv)
>  {
>  	OptionsParser::Options options;
> @@ -103,6 +114,16 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
>  		}
>  
>  		options.values_[c] = optarg ? optarg : "";
> +
> +		/* Parse argument as key=values if needed. */
> +		if (optionsMap_[c].keyValueParser) {
> +			options.keyValues_[c] = optionsMap_[c].keyValueParser->parse(options.values_[c].c_str());
> +			if (!options.keyValues_[c].valid()) {
> +				usage();
> +				options.clear();
> +				break;
> +			}
> +		}
>  	}
>  
>  	return options;
> @@ -169,5 +190,141 @@ void OptionsParser::usage()
>  				std::cerr << help << std::endl;
>  			}
>  		}
> +
> +		if (option.keyValueParser)
> +			option.keyValueParser->usage(indent);
> +	}
> +}
> +
> +bool OptionsParser::Options::isKeyValue(int opt)
> +{
> +	return keyValues_.find(opt) != keyValues_.end();
> +}
> +
> +KeyValueParser::Options OptionsParser::Options::keyValues(int opt)
> +{
> +	return keyValues_.find(opt)->second;
> +}
> +
> +void KeyValueParser::addOption(const char *name,
> +			       const char *help,
> +			       OptionArgument argument,
> +			       const char *argumentName)
> +{
> +	if (!name)
> +		return;
> +	if (!help || help[0] == '\0')
> +		return;
> +	if (argument != ArgumentNone && !argumentName)
> +		return;
> +
> +	/* Reject duplicate options. */
> +	if (optionsMap_.find(name) != optionsMap_.end())
> +		return;
> +
> +	optionsMap_[name] = Option({ name, help, argument, argumentName });
> +}
> +
> +KeyValueParser::Options KeyValueParser::parse(const char *arguments)
> +{
> +	Options options;
> +
> +	for (const char *pair = arguments; *arguments != '\0'; pair = arguments) {
> +		const char *comma = strchrnul(arguments, ',');
> +		size_t len = comma - pair;
> +
> +		/* Skip over the comma. */
> +		arguments = *comma == ',' ? comma + 1 : comma;
> +
> +		/* Skip to the next pair if the pair is empty. */
> +		if (!len)
> +			continue;
> +
> +		std::string key;
> +		std::string value;
> +
> +		const char *separator = static_cast<const char *>(memchr(pair, '=', len));
> +		if (!separator) {
> +			key = std::string(pair, len);
> +			value = "";
> +		} else {
> +			key = std::string(pair, separator - pair);
> +			value = std::string(separator + 1, comma - separator - 1);
> +		}
> +
> +		/* The key is mandatory, the value might be optional. */
> +		if (key.empty())
> +			continue;
> +
> +		if (optionsMap_.find(key) == optionsMap_.end()) {
> +			std::cerr << "Invalid option " << key << std::endl;
> +			options.clear();
> +			break;
> +		}
> +
> +		if (value == "" && optionsMap_[key].argument == ArgumentRequired) {
> +			std::cerr << "Missing argument for option " << key << std::endl;
> +			options.clear();
> +			break;
> +		}
> +
> +		if (value != "" && optionsMap_[key].argument == ArgumentNone) {
> +			std::cerr << "Argument specified for option " << key << std::endl;
> +			options.clear();
> +			break;
> +		}

How about

		OptionArgument arg = optionsMap_[key].argument;
		if (value.empty() && arg == ArgumentRequired) {
			std::cerr << "Option " << key << " requires an argument" << std::endl;
			options.clear();
			break;
		} else if (!value.empty() && arg == ArgumentNone) {
			std::cerr << "Option " << key << " doesn't take an argument" << std::endl;
			options.clear();
			break;
		}

> +		options.values_[key] = value;
> +	}
> +
> +	return options;
> +}
> +
> +void KeyValueParser::usage(int indent)
> +{
> +	unsigned int space = 0;
> +
> +	for (auto const &iter : optionsMap_) {
> +		const Option &option = iter.second;
> +		unsigned int length = 14;
> +		if (option.argument != ArgumentNone)
> +			length += 1 + strlen(option.argumentName);
> +		if (option.argument == ArgumentOptional)
> +			length += 2;
> +
> +		if (length > space)
> +			space = length;
> +	}
> +
> +	space = (space + 7) / 8 * 8;
> +
> +	for (auto const &iter : optionsMap_) {
> +		const Option &option = iter.second;
> +

No need for a blank line.

> +		std::string argument = option.name;
> +
> +		if (option.argument != ArgumentNone) {
> +			if (option.argument == ArgumentOptional)
> +				argument += "[=";
> +			else
> +				argument += "=";
> +			argument += option.argumentName;
> +			if (option.argument == ArgumentOptional)
> +				argument += "]";
> +		}
> +
> +		std::cerr << std::setw(indent) << std::right << " "
> +			  << std::setw(space) << std::left << argument;
> +
> +		for (const char *help = option.help, *end = help; end;) {
> +			end = strchr(help, '\n');
> +			if (end) {
> +				std::cerr << std::string(help, end - help + 1);
> +				std::cerr << std::setw(indent + space) << " ";
> +				help = end + 1;
> +			} else {
> +				std::cerr << help << std::endl;
> +			}
> +		}
>  	}
>  }
> diff --git a/src/cam/options.h b/src/cam/options.h
> index cb7286a0a8005579..91a78406a601d737 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -27,20 +27,54 @@ public:
>  
>  private:
>  	friend class OptionsParser;
> +	friend class KeyValueParser;

I'd sort there alphabetically.

>  	std::map<T, std::string> values_;
>  	void clear() { values_.clear(); };
>  };
>  
> +class KeyValueParser
> +{
> +public:
> +	class Options : public OptionsBase<std::string>
> +	{
> +	};
> +
> +	void addOption(const char *name, const char *help,
> +		       OptionArgument argument = ArgumentNone,
> +		       const char *argumentName = nullptr);
> +
> +	Options parse(const char *arguments);
> +	void usage(int indent);
> +
> +private:
> +	struct Option {
> +		const char *name;
> +		const char *help;
> +		OptionArgument argument;
> +		const char *argumentName;
> +	};
> +
> +	std::map<std::string, Option> optionsMap_;

Maybe just options_ ?

> +};
> +
>  class OptionsParser
>  {
>  public:
>  	class Options : public OptionsBase<int>
>  	{
> +	public:
> +		bool isKeyValue(int opt);
> +		KeyValueParser::Options keyValues(int opt);
> +	private:
> +		friend class OptionsParser;
> +		std::map<unsigned int, KeyValueParser::Options> keyValues_;
>  	};
>  
>  	bool addOption(int opt, const char *help, const char *name = nullptr,
>  		       OptionArgument argument = ArgumentNone,
>  		       const char *argumentName = nullptr);
> +	void addOption(int opt, const char *help, KeyValueParser &parser,
> +		       const char *name = nullptr);

Shouldn't this return a bool like the other addOption() ?

>  	Options parse(int argc, char *argv[]);
>  	void usage();
> @@ -52,6 +86,7 @@ private:
>  		OptionArgument argument;
>  		const char *argumentName;
>  		const char *help;
> +		KeyValueParser *keyValueParser;
>  
>  		bool hasShortOption() const { return isalnum(opt); }
>  		bool hasLongOption() const { return name != nullptr; }