[{"id":629,"web_url":"https://patchwork.libcamera.org/comment/629/","msgid":"<20190126170920.GB4542@pendragon.ideasonboard.com>","date":"2019-01-26T17:09:20","subject":"Re: [libcamera-devel] [RFC 1/2] cam: options: add parser for\n\tsuboptions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Jan 25, 2019 at 10:21:53PM +0100, Niklas Söderlund wrote:\n> The cam utility will make use of lists of key/value pairs from the\n> command line arguments. This is needed so that a user can among other\n> things describe complex stream format descriptions, which consists of a\n> set of parameters.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/options.cpp | 68 +++++++++++++++++++++++++++++++++++++++++++++\n> src/cam/options.h | 24 ++++++++++++++++\n> 2 files changed, 92 insertions(+)\n> \n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> index 55c42540f92478e6..3299cdfed69703fa 100644\n> --- a/src/cam/options.cpp\n> +++ b/src/cam/options.cpp\n> @@ -180,3 +180,71 @@ void OptionsParser::Options::clear()\n> {\n> \tvalues_.clear();\n> }\n> +\n> +void SubOptionsParser::addToken(const char *name)\n\nI was going to propose calling this KeyValueParser, and then realized\nthere was a getsubopt() function in glibc. As the class isn't restricted\nto suboptions but could parse any kind of key/value pairs, I think the\nname KeyValueParser may still be better.\n\nShould this function take a boolean that specifies if the value is\nmandatory ? How about a help text, as in the options parser ? You could\neven add a new addOption() overload to OptionsParser that would take a\nsuboption parser as an argument to automate all this ?\n\n> +{\n> +\tif (!name)\n> +\t\treturn;\n> +\n> +\t/* Reject duplicate options. */\n> +\tfor (const char *option : options_)\n> +\t\tif (!strcmp(name, option))\n> +\t\t\treturn;\n> +\n> +\toptions_.push_back(name);\n\nYou could store them in a map or unordered_set to ensure uniqueness of\nthe name.\n\n> +}\n> +\n> +SubOptionsParser::Options SubOptionsParser::parse(const char *argc)\n\ns/argc/options/ ?\n\n> +{\n> +\tconst char **tokens;\n> +\tchar *dupe, *subs, *value;\n> +\tOptions options;\n> +\n> +\ttokens = new const char *[options_.size() + 1]();\n> +\tfor (unsigned int i = 0; i < options_.size(); i++)\n> +\t\ttokens[i] = options_.at(i);\n> +\n> +\tdupe = strdup(argc);\n> +\tsubs = dupe;\n> +\twhile (*subs != '\\0') {\n> +\t\tint opt = getsubopt(&subs, (char *const *)tokens, &value);\n> +\n> +\t\tif (opt == -1 || !value || value[0] == '\\0') {\n> +\t\t\tif (opt == -1)\n> +\t\t\t\tstd::cerr << \"Invalid option \" << value << std::endl;\n> +\t\t\telse\n> +\t\t\t\tstd::cerr << \"Missing argument for option \"\n> +\t\t\t\t\t << tokens[opt] << std::endl;\n> +\n> +\t\t\toptions.clear();\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\toptions.values_[tokens[opt]] = value;\n> +\t}\n> +\n> +\tfree(dupe);\n> +\tdelete[] tokens;\n\nThis looks pretty complex. Wouldn't it be simpler to hand-code this ?\nYou could copy the code from Logger::parseLogLevels().\n\n for (const char *pair = options; *options != '\\0'; pair = options) {\n const char *comma = strchrnul(options, ',');\n size_t len = comma - pair;\n\n /* Skip over the comma. */\n options = *comma == ',' ? comma + 1 : comma;\n\n /* Skip to the next pair if the pair is empty. */\n if (!len)\n continue;\n\n std::string key;\n std::string value;\n\n const char *colon = static_cast(memchr(pair, ':', len));\n if (!colon) {\n key = std::string(pair, len);\n value = \"\";\n } else {\n key = std::string(pair, colon - pair);\n value = std::string(colon + 1, comma - colon - 1);\n }\n\n /* The key is mandatory, the value is optional. */\n if (key.empty())\n continue;\n\n option.values_[key] = value;\n }\n\n(You probably want to return an error instead of continuing if the string\nisn't valid)\n\nUp to you.\n\n> +\treturn options;\n> +}\n> +\n> +bool SubOptionsParser::Options::valid() const\n> +{\n> +\treturn !values_.empty();\n> +}\n> +\n> +bool SubOptionsParser::Options::isSet(std::string opt) const\n> +{\n> +\treturn values_.find(opt) != values_.end();\n> +}\n> +\n> +const std::string &SubOptionsParser::Options::operator[](std::string opt) const\n> +{\n> +\treturn values_.find(opt)->second;\n> +}\n> +\n> +void SubOptionsParser::Options::clear()\n> +{\n> +\tvalues_.clear();\n> +}\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> index f99ea7300a71c24f..653dda0c76e52251 100644\n> --- a/src/cam/options.h\n> +++ b/src/cam/options.h\n> @@ -57,4 +57,28 @@ private:\n> \tstd::map optionsMap_;\n> };\n> \n> +class SubOptionsParser\n> +{\n> +public:\n> +\tclass Options\n> +\t{\n> +\tpublic:\n> +\t\tbool valid() const;\n> +\t\tbool isSet(std::string opt) const;\n\nconst std::string &opt\n\n> +\t\tconst std::string &operator[](std::string opt) const;\n\nconst std::string &opt\n\n> +\n> +\tprivate:\n> +\t\tfriend class SubOptionsParser;\n> +\t\tstd::map values_;\n> +\t\tvoid clear();\n> +\t};\n> +\n> +\tvoid addToken(const char *token);\n> +\n> +\tOptions parse(const char *argc);\n> +\n> +private:\n> +\tstd::vector options_;\n> +};\n> +\n> #endif /* __CAM_OPTIONS_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CD2760B2D\n\tfor ;\n\tSat, 26 Jan 2019 18:09:29 +0100 (CET)","from pendragon.ideasonboard.com (85-76-45-169-nat.elisa-mobile.fi\n\t[85.76.45.169])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C499A23A;\n\tSat, 26 Jan 2019 18:09:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548522569;\n\tbh=5ptbNDXvZsAqbV2Ge+5IwHP6ykFvxLc10vCBUbOiqOg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iOM59SqkZzEFVIm2HnuSLweHPJhndpE0D0lWDYXWAsAJrmisx8EWOqVG55gReZAhT\n\tmy4UK0Ii9QjfnaLhsAdAQ+9OFcqEinwseCdj4qwq9M7hz1l5sa/d4qY909Hq5HICOV\n\tBT1c90j/5NXb4e+WX5uAzUkBTbOt59lC6WfM6rjk=","Date":"Sat, 26 Jan 2019 19:09:20 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190126170920.GB4542@pendragon.ideasonboard.com>","References":"<20190125212154.26950-1-niklas.soderlund@ragnatech.se>\n\t<20190125212154.26950-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190125212154.26950-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 1/2] cam: options: add parser for\n\tsuboptions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 26 Jan 2019 17:09:29 -0000"}}]