[libcamera-devel] [RFC 1/2] cam: options: add parser for suboptions

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 25, 2019 at 10:21:53PM +0100, Niklas Söderlund wrote:
> The cam utility will make use of lists of key/value pairs from the
> command line arguments. This is needed so that a user can among other
> things describe complex stream format descriptions, which consists of a
> set of parameters.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/options.cpp | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  src/cam/options.h   | 24 ++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 55c42540f92478e6..3299cdfed69703fa 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -180,3 +180,71 @@ void OptionsParser::Options::clear()
>  {
>  	values_.clear();
>  }
> +
> +void SubOptionsParser::addToken(const char *name)

I was going to propose calling this KeyValueParser, and then realized
there was a getsubopt() function in glibc. As the class isn't restricted
to suboptions but could parse any kind of key/value pairs, I think the
name KeyValueParser may still be better.

Should this function take a boolean that specifies if the value is
mandatory ? How about a help text, as in the options parser ? You could
even add a new addOption() overload to OptionsParser that would take a
suboption parser as an argument to automate all this ?

> +{
> +	if (!name)
> +		return;
> +
> +	/* Reject duplicate options. */
> +	for (const char *option : options_)
> +		if (!strcmp(name, option))
> +			return;
> +
> +	options_.push_back(name);

You could store them in a map or unordered_set to ensure uniqueness of
the name.

> +}
> +
> +SubOptionsParser::Options SubOptionsParser::parse(const char *argc)

s/argc/options/ ?

> +{
> +	const char **tokens;
> +	char *dupe, *subs, *value;
> +	Options options;
> +
> +	tokens = new const char *[options_.size() + 1]();
> +	for (unsigned int i = 0; i < options_.size(); i++)
> +		tokens[i] = options_.at(i);
> +
> +	dupe = strdup(argc);
> +	subs = dupe;
> +	while (*subs != '\0') {
> +		int opt = getsubopt(&subs, (char *const *)tokens, &value);
> +
> +		if (opt == -1 || !value || value[0] == '\0') {
> +			if (opt == -1)
> +				std::cerr << "Invalid option " << value << std::endl;
> +			else
> +				std::cerr << "Missing argument for option "
> +					  << tokens[opt] << std::endl;
> +
> +			options.clear();
> +			break;
> +		}
> +
> +		options.values_[tokens[opt]] = value;
> +	}
> +
> +	free(dupe);
> +	delete[] tokens;

This looks pretty complex. Wouldn't it be simpler to hand-code this ?
You could copy the code from Logger::parseLogLevels().

        for (const char *pair = options; *options != '\0'; pair = options) {
                const char *comma = strchrnul(options, ',');
                size_t len = comma - pair;

                /* Skip over the comma. */
                options = *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 *colon = static_cast<const char *>(memchr(pair, ':', len));
                if (!colon) {
                        key = std::string(pair, len);
                        value = "";
                } else {
                        key = std::string(pair, colon - pair);
                        value = std::string(colon + 1, comma - colon - 1);
                }

                /* The key is mandatory, the value is optional. */
                if (key.empty())
                        continue;

                option.values_[key] = value;
        }

(You probably want to return an error instead of continuing if the string
isn't valid)

Up to you.

> +	return options;
> +}
> +
> +bool SubOptionsParser::Options::valid() const
> +{
> +	return !values_.empty();
> +}
> +
> +bool SubOptionsParser::Options::isSet(std::string opt) const
> +{
> +	return values_.find(opt) != values_.end();
> +}
> +
> +const std::string &SubOptionsParser::Options::operator[](std::string opt) const
> +{
> +	return values_.find(opt)->second;
> +}
> +
> +void SubOptionsParser::Options::clear()
> +{
> +	values_.clear();
> +}
> diff --git a/src/cam/options.h b/src/cam/options.h
> index f99ea7300a71c24f..653dda0c76e52251 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -57,4 +57,28 @@ private:
>  	std::map<unsigned int, Option *> optionsMap_;
>  };
>  
> +class SubOptionsParser
> +{
> +public:
> +	class Options
> +	{
> +	public:
> +		bool valid() const;
> +		bool isSet(std::string opt) const;

const std::string &opt

> +		const std::string &operator[](std::string opt) const;

const std::string &opt

> +
> +	private:
> +		friend class SubOptionsParser;
> +		std::map<std::string, std::string> values_;
> +		void clear();
> +	};
> +
> +	void addToken(const char *token);
> +
> +	Options parse(const char *argc);
> +
> +private:
> +	std::vector<const char *> options_;
> +};
> +
>  #endif /* __CAM_OPTIONS_H__ */

-- 
Regards,

Laurent Pinchart