[{"id":721,"web_url":"https://patchwork.libcamera.org/comment/721/","msgid":"<20190131105331.GJ4197@pendragon.ideasonboard.com>","date":"2019-01-31T10:53:31","subject":"Re: [libcamera-devel] [PATCH 5/6] cam: options: add a key=value\n\tparser","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 Mon, Jan 28, 2019 at 01:41:08AM +0100, Niklas Söderlund wrote:\n> Some options passed to the cam utility needs to be complex and specify a\n\ns/needs/need/\n\n> list of key=value pairs, add a new parser to deal white these options.\n\ns/, add/. Add/\ns/white/with/\n\n> The new parser is integrated into the existing OptionsParser and the cam\n> application can fully describe all its options in one location and\n> perform full parsing of all arguments in one go. The new key=value\n> parsers also integrates itself with the usage() printing of the\n> OptionsParser.\n> \n> The new parser can also be used on it's own to parse key=value pairs\n\ns/it's/its/\n\n> from different data sources then the command line options.\n\ns/then/than/\n\nI would have split this patch in two, with one patch to add the\nkey-value parser, and another patch to integrate it in the options\nparser.\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/cam/options.cpp | 159 +++++++++++++++++++++++++++++++++++++++++++-\n>  src/cam/options.h   |  35 ++++++++++\n>  2 files changed, 193 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> index d3bff1cd897a5cfb..f63fd3b495e29986 100644\n> --- a/src/cam/options.cpp\n> +++ b/src/cam/options.cpp\n> @@ -30,11 +30,22 @@ bool OptionsParser::addOption(int opt, const char *help, const char *name,\n>  \tif (optionsMap_.find(opt) != optionsMap_.end())\n>  \t\treturn false;\n>  \n> -\toptionsMap_[opt] = Option({ opt, name, argument, argumentName, help });\n> +\toptionsMap_[opt] = Option({ opt, name, argument, argumentName, help,\n> +\t\t\t\t    nullptr });\n>  \n>  \treturn true;\n>  }\n>  \n> +void OptionsParser::addOption(int opt, const char *help, KeyValueParser &parser,\n> +\t\t\t      const char *name)\n> +{\n> +\tif (!addOption(opt, help, name, ArgumentRequired,\n> +\t\t       \"key=value[,key=value,...]\"))\n> +\t\treturn;\n> +\n> +\toptionsMap_[opt].keyValueParser = &parser;\n> +}\n> +\n>  OptionsParser::Options OptionsParser::parse(int argc, char **argv)\n>  {\n>  \tOptionsParser::Options options;\n> @@ -103,6 +114,16 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)\n>  \t\t}\n>  \n>  \t\toptions.values_[c] = optarg ? optarg : \"\";\n> +\n> +\t\t/* Parse argument as key=values if needed. */\n> +\t\tif (optionsMap_[c].keyValueParser) {\n> +\t\t\toptions.keyValues_[c] = optionsMap_[c].keyValueParser->parse(options.values_[c].c_str());\n> +\t\t\tif (!options.keyValues_[c].valid()) {\n> +\t\t\t\tusage();\n> +\t\t\t\toptions.clear();\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n>  \t}\n>  \n>  \treturn options;\n> @@ -169,5 +190,141 @@ void OptionsParser::usage()\n>  \t\t\t\tstd::cerr << help << std::endl;\n>  \t\t\t}\n>  \t\t}\n> +\n> +\t\tif (option.keyValueParser)\n> +\t\t\toption.keyValueParser->usage(indent);\n> +\t}\n> +}\n> +\n> +bool OptionsParser::Options::isKeyValue(int opt)\n> +{\n> +\treturn keyValues_.find(opt) != keyValues_.end();\n> +}\n> +\n> +KeyValueParser::Options OptionsParser::Options::keyValues(int opt)\n> +{\n> +\treturn keyValues_.find(opt)->second;\n> +}\n> +\n> +void KeyValueParser::addOption(const char *name,\n> +\t\t\t       const char *help,\n> +\t\t\t       OptionArgument argument,\n> +\t\t\t       const char *argumentName)\n> +{\n> +\tif (!name)\n> +\t\treturn;\n> +\tif (!help || help[0] == '\\0')\n> +\t\treturn;\n> +\tif (argument != ArgumentNone && !argumentName)\n> +\t\treturn;\n> +\n> +\t/* Reject duplicate options. */\n> +\tif (optionsMap_.find(name) != optionsMap_.end())\n> +\t\treturn;\n> +\n> +\toptionsMap_[name] = Option({ name, help, argument, argumentName });\n> +}\n> +\n> +KeyValueParser::Options KeyValueParser::parse(const char *arguments)\n> +{\n> +\tOptions options;\n> +\n> +\tfor (const char *pair = arguments; *arguments != '\\0'; pair = arguments) {\n> +\t\tconst char *comma = strchrnul(arguments, ',');\n> +\t\tsize_t len = comma - pair;\n> +\n> +\t\t/* Skip over the comma. */\n> +\t\targuments = *comma == ',' ? comma + 1 : comma;\n> +\n> +\t\t/* Skip to the next pair if the pair is empty. */\n> +\t\tif (!len)\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::string key;\n> +\t\tstd::string value;\n> +\n> +\t\tconst char *separator = static_cast<const char *>(memchr(pair, '=', len));\n> +\t\tif (!separator) {\n> +\t\t\tkey = std::string(pair, len);\n> +\t\t\tvalue = \"\";\n> +\t\t} else {\n> +\t\t\tkey = std::string(pair, separator - pair);\n> +\t\t\tvalue = std::string(separator + 1, comma - separator - 1);\n> +\t\t}\n> +\n> +\t\t/* The key is mandatory, the value might be optional. */\n> +\t\tif (key.empty())\n> +\t\t\tcontinue;\n> +\n> +\t\tif (optionsMap_.find(key) == optionsMap_.end()) {\n> +\t\t\tstd::cerr << \"Invalid option \" << key << std::endl;\n> +\t\t\toptions.clear();\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (value == \"\" && optionsMap_[key].argument == ArgumentRequired) {\n> +\t\t\tstd::cerr << \"Missing argument for option \" << key << std::endl;\n> +\t\t\toptions.clear();\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (value != \"\" && optionsMap_[key].argument == ArgumentNone) {\n> +\t\t\tstd::cerr << \"Argument specified for option \" << key << std::endl;\n> +\t\t\toptions.clear();\n> +\t\t\tbreak;\n> +\t\t}\n\nHow about\n\n\t\tOptionArgument arg = optionsMap_[key].argument;\n\t\tif (value.empty() && arg == ArgumentRequired) {\n\t\t\tstd::cerr << \"Option \" << key << \" requires an argument\" << std::endl;\n\t\t\toptions.clear();\n\t\t\tbreak;\n\t\t} else if (!value.empty() && arg == ArgumentNone) {\n\t\t\tstd::cerr << \"Option \" << key << \" doesn't take an argument\" << std::endl;\n\t\t\toptions.clear();\n\t\t\tbreak;\n\t\t}\n\n> +\t\toptions.values_[key] = value;\n> +\t}\n> +\n> +\treturn options;\n> +}\n> +\n> +void KeyValueParser::usage(int indent)\n> +{\n> +\tunsigned int space = 0;\n> +\n> +\tfor (auto const &iter : optionsMap_) {\n> +\t\tconst Option &option = iter.second;\n> +\t\tunsigned int length = 14;\n> +\t\tif (option.argument != ArgumentNone)\n> +\t\t\tlength += 1 + strlen(option.argumentName);\n> +\t\tif (option.argument == ArgumentOptional)\n> +\t\t\tlength += 2;\n> +\n> +\t\tif (length > space)\n> +\t\t\tspace = length;\n> +\t}\n> +\n> +\tspace = (space + 7) / 8 * 8;\n> +\n> +\tfor (auto const &iter : optionsMap_) {\n> +\t\tconst Option &option = iter.second;\n> +\n\nNo need for a blank line.\n\n> +\t\tstd::string argument = option.name;\n> +\n> +\t\tif (option.argument != ArgumentNone) {\n> +\t\t\tif (option.argument == ArgumentOptional)\n> +\t\t\t\targument += \"[=\";\n> +\t\t\telse\n> +\t\t\t\targument += \"=\";\n> +\t\t\targument += option.argumentName;\n> +\t\t\tif (option.argument == ArgumentOptional)\n> +\t\t\t\targument += \"]\";\n> +\t\t}\n> +\n> +\t\tstd::cerr << std::setw(indent) << std::right << \" \"\n> +\t\t\t  << std::setw(space) << std::left << argument;\n> +\n> +\t\tfor (const char *help = option.help, *end = help; end;) {\n> +\t\t\tend = strchr(help, '\\n');\n> +\t\t\tif (end) {\n> +\t\t\t\tstd::cerr << std::string(help, end - help + 1);\n> +\t\t\t\tstd::cerr << std::setw(indent + space) << \" \";\n> +\t\t\t\thelp = end + 1;\n> +\t\t\t} else {\n> +\t\t\t\tstd::cerr << help << std::endl;\n> +\t\t\t}\n> +\t\t}\n>  \t}\n>  }\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> index cb7286a0a8005579..91a78406a601d737 100644\n> --- a/src/cam/options.h\n> +++ b/src/cam/options.h\n> @@ -27,20 +27,54 @@ public:\n>  \n>  private:\n>  \tfriend class OptionsParser;\n> +\tfriend class KeyValueParser;\n\nI'd sort there alphabetically.\n\n>  \tstd::map<T, std::string> values_;\n>  \tvoid clear() { values_.clear(); };\n>  };\n>  \n> +class KeyValueParser\n> +{\n> +public:\n> +\tclass Options : public OptionsBase<std::string>\n> +\t{\n> +\t};\n> +\n> +\tvoid addOption(const char *name, const char *help,\n> +\t\t       OptionArgument argument = ArgumentNone,\n> +\t\t       const char *argumentName = nullptr);\n> +\n> +\tOptions parse(const char *arguments);\n> +\tvoid usage(int indent);\n> +\n> +private:\n> +\tstruct Option {\n> +\t\tconst char *name;\n> +\t\tconst char *help;\n> +\t\tOptionArgument argument;\n> +\t\tconst char *argumentName;\n> +\t};\n> +\n> +\tstd::map<std::string, Option> optionsMap_;\n\nMaybe just options_ ?\n\n> +};\n> +\n>  class OptionsParser\n>  {\n>  public:\n>  \tclass Options : public OptionsBase<int>\n>  \t{\n> +\tpublic:\n> +\t\tbool isKeyValue(int opt);\n> +\t\tKeyValueParser::Options keyValues(int opt);\n> +\tprivate:\n> +\t\tfriend class OptionsParser;\n> +\t\tstd::map<unsigned int, KeyValueParser::Options> keyValues_;\n>  \t};\n>  \n>  \tbool addOption(int opt, const char *help, const char *name = nullptr,\n>  \t\t       OptionArgument argument = ArgumentNone,\n>  \t\t       const char *argumentName = nullptr);\n> +\tvoid addOption(int opt, const char *help, KeyValueParser &parser,\n> +\t\t       const char *name = nullptr);\n\nShouldn't this return a bool like the other addOption() ?\n\n>  \tOptions parse(int argc, char *argv[]);\n>  \tvoid usage();\n> @@ -52,6 +86,7 @@ private:\n>  \t\tOptionArgument argument;\n>  \t\tconst char *argumentName;\n>  \t\tconst char *help;\n> +\t\tKeyValueParser *keyValueParser;\n>  \n>  \t\tbool hasShortOption() const { return isalnum(opt); }\n>  \t\tbool hasLongOption() const { return name != nullptr; }","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3D7760B10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 31 Jan 2019 11:53:35 +0100 (CET)","from pendragon.ideasonboard.com (85-76-34-136-nat.elisa-mobile.fi\n\t[85.76.34.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E50541;\n\tThu, 31 Jan 2019 11:53:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548932015;\n\tbh=aIMTawwBnbXzGXlCzUVyOsZnDhD8VCXyuhqR4LO16QM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=atixBIMOwBBkBrH0KFCS4qVeu402Y/7EvaMXACAQ6MgLBstdF1p3w4vUsG7Nko6VT\n\tYjSCVlkaycFneRq9GVFLYwxvGtgBOD3KwjHh4TLU2TZraN0Iiz5f2nfDHtrLdbx5eJ\n\tNw+NctGRrHYGtGrfPeq7ljk0rAXRV1trD/D8LXps=","Date":"Thu, 31 Jan 2019 12:53:31 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190131105331.GJ4197@pendragon.ideasonboard.com>","References":"<20190128004109.25860-1-niklas.soderlund@ragnatech.se>\n\t<20190128004109.25860-6-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":"<20190128004109.25860-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/6] cam: options: add a key=value\n\tparser","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 31 Jan 2019 10:53:36 -0000"}}]