[{"id":487,"web_url":"https://patchwork.libcamera.org/comment/487/","msgid":"<20190122130753.GP6484@bigcity.dyn.berto.se>","date":"2019-01-22T13:07:53","subject":"Re: [libcamera-devel] [PATCH] cam: Extract option parser to\n\tseparate file","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello,\n\nThese two patches have been merged.\n\nOn 2019-01-22 05:03:54 +0200, Laurent Pinchart wrote:\n> And turn it into an OptionsParser object.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n> \n> Hi Niklas,\n> \n> On top of your cam utility patch, a bit of cleanup and argument parsing\n> refactoring. With this on top the base patch is good for me, feel free\n> to push the combination (in which case please don't forget to add your\n> SoB to this patch) - possibly after waiting for more review.\n> \n>  src/cam/{cam.cpp => main.cpp} | 117 +++++----------------\n>  src/cam/meson.build           |   3 +-\n>  src/cam/options.cpp           | 192 ++++++++++++++++++++++++++++++++++\n>  src/cam/options.h             |  62 +++++++++++\n>  4 files changed, 285 insertions(+), 89 deletions(-)\n>  rename src/cam/{cam.cpp => main.cpp} (22%)\n>  create mode 100644 src/cam/options.cpp\n>  create mode 100644 src/cam/options.h\n> \n> diff --git a/src/cam/cam.cpp b/src/cam/main.cpp\n> similarity index 22%\n> rename from src/cam/cam.cpp\n> rename to src/cam/main.cpp\n> index 0f795be78106..22211670c625 100644\n> --- a/src/cam/cam.cpp\n> +++ b/src/cam/main.cpp\n> @@ -2,139 +2,80 @@\n>  /*\n>   * Copyright (C) 2019, Google Inc.\n>   *\n> - * main.cpp - cam-ctl a tool to interact with the library\n> + * main.cpp - cam - The libcamera swiss army knife\n>   */\n>  \n> -#include <getopt.h>\n> -#include <iomanip>\n>  #include <iostream>\n>  #include <map>\n>  #include <string.h>\n>  \n>  #include <libcamera/libcamera.h>\n>  \n> -#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> +#include \"options.h\"\n>  \n> -using namespace std;\n>  using namespace libcamera;\n>  \n> -enum Option {\n> +OptionsParser::Options options;\n> +\n> +enum {\n>  \tOptCamera = 'c',\n>  \tOptHelp = 'h',\n>  \tOptList = 'l',\n> -\tOptLast = 0,\n> -};\n> -\n> -struct OptionInfo {\n> -\tOption id;\n> -\tconst char *name;\n> -\tconst char *arguments;\n> -\tconst char *description;\n>  };\n>  \n> -static struct OptionInfo option_info[] = {\n> -\t{ OptCamera, \"camera\", \"<camera>\", \"Specify which camera to operate on\" },\n> -\t{ OptHelp, \"help\", nullptr, \"Display this help message\" },\n> -\t{ OptList, \"list\", nullptr, \"List all cameras\" },\n> -\t{ OptLast, nullptr, nullptr, nullptr },\n> -};\n> -\n> -std::map<Option, std::string> options;\n> -\n> -void usage()\n> +static int parseOptions(int argc, char *argv[])\n>  {\n> -\tstruct OptionInfo *info;\n> +\tOptionsParser parser;\n>  \n> -\tcout << \"Options:\" << endl;\n> -\tfor (info = option_info; info->id != OptLast; info++) {\n> -\t\tstring arg(info->name);\n> +\tparser.addOption(OptCamera, \"Specify which camera to operate on\",\n> +\t\t\t \"camera\", OptionsParser::ArgumentRequired,\n> +\t\t\t \"camera\");\n> +\tparser.addOption(OptHelp, \"Display this help message\", \"help\");\n> +\tparser.addOption(OptList, \"List all cameras\", \"list\");\n>  \n> -\t\tif (info->arguments)\n> -\t\t\targ += string(\" \") + info->arguments;\n> +\toptions = std::move(parser.parse(argc, argv));\n> +\tif (!options.valid())\n> +\t\treturn -EINVAL;\n>  \n> -\t\tcout << \"  -\" << static_cast<char>(info->id) << \" --\" <<\n> -\t\t\tsetw(20) << left << arg << \" - \" <<\n> -\t\t\tinfo->description << endl;\n> -\t}\n> -}\n> -\n> -int parseOptions(int argc, char **argv)\n> -{\n> -\tchar short_options[ARRAY_SIZE(option_info) * 2 + 1];\n> -\tstruct option long_options[ARRAY_SIZE(option_info)];\n> -\tstruct OptionInfo *info;\n> -\tunsigned ids = 0, idl = 0;\n> -\n> -\tmemset(short_options, 0, sizeof(short_options));\n> -\tmemset(long_options, 0, sizeof(long_options));\n> -\n> -\tfor (info = option_info; info->id != OptLast; info++) {\n> -\t\tshort_options[ids++] = info->id;\n> -\t\tif (info->arguments)\n> -\t\t\tshort_options[ids++] = ':';\n> -\n> -\t\tlong_options[idl].name = info->name;\n> -\t\tlong_options[idl].has_arg =\n> -\t\t\tinfo->arguments ? required_argument : no_argument;\n> -\t\tlong_options[idl].flag = 0;\n> -\t\tlong_options[idl].val = info->id;\n> -\t\tidl++;\n> -\t}\n> -\n> -\twhile (true) {\n> -\t\tint c = getopt_long(argc, argv, short_options, long_options, nullptr);\n> -\n> -\t\tif (c == -1)\n> -\t\t\tbreak;\n> -\n> -\t\tif (!isalpha(c))\n> -\t\t\treturn EXIT_FAILURE;\n> -\n> -\t\toptions[static_cast<Option>(c)] = optarg ? string(optarg) : \"\";\n> +\tif (argc == 1 || options.isSet(OptHelp)) {\n> +\t\tparser.usage();\n> +\t\treturn 1;\n>  \t}\n>  \n>  \treturn 0;\n>  }\n>  \n> -bool optSet(Option opt)\n> -{\n> -\treturn options.count(opt) != 0;\n> -}\n> -\n>  int main(int argc, char **argv)\n>  {\n>  \tint ret;\n>  \n>  \tret = parseOptions(argc, argv);\n> -\tif (ret == EXIT_FAILURE)\n> -\t\treturn ret;\n> -\n> -\tif (argc == 1 || optSet(OptHelp)) {\n> -\t\tusage();\n> -\t\treturn 0;\n> -\t}\n> +\tif (ret < 0)\n> +\t\treturn EXIT_FAILURE;\n>  \n>  \tCameraManager *cm = CameraManager::instance();\n>  \n>  \tret = cm->start();\n>  \tif (ret) {\n> -\t\tcout << \"Failed to start camera manager: \" << strerror(-ret) << endl;\n> +\t\tstd::cout << \"Failed to start camera manager: \"\n> +\t\t\t  << strerror(-ret) << std::endl;\n>  \t\treturn EXIT_FAILURE;\n>  \t}\n>  \n> -\tif (optSet(OptList)) {\n> -\t\tcout << \"Available cameras:\" << endl;\n> +\tif (options.isSet(OptList)) {\n> +\t\tstd::cout << \"Available cameras:\" << std::endl;\n>  \t\tfor (const std::shared_ptr<Camera> &camera : cm->cameras())\n> -\t\t\tcout << \"- \" << camera->name() << endl;\n> +\t\t\tstd::cout << \"- \" << camera->name() << std::endl;\n>  \t}\n>  \n> -\tif (optSet(OptCamera)) {\n> +\tif (options.isSet(OptCamera)) {\n>  \t\tstd::shared_ptr<Camera> cam = cm->get(options[OptCamera]);\n>  \n>  \t\tif (cam) {\n> -\t\t\tcout << \"Using camera \" << cam->name() << endl;\n> +\t\t\tstd::cout << \"Using camera \" << cam->name() << std::endl;\n>  \t\t} else {\n> -\t\t\tcout << \"Camera \" << options[OptCamera] << \" not found\" << endl;\n> +\t\t\tstd::cout << \"Camera \" << options[OptCamera]\n> +\t\t\t\t  << \" not found\" << std::endl;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 809a40e03492..e45e5391f679 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -1,5 +1,6 @@\n>  cam_sources = files([\n> -    'cam.cpp',\n> +    'main.cpp',\n> +    'options.cpp',\n>  ])\n>  \n>  cam  = executable('cam', cam_sources,\n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> new file mode 100644\n> index 000000000000..d391a0e58436\n> --- /dev/null\n> +++ b/src/cam/options.cpp\n> @@ -0,0 +1,192 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * options.cpp - cam - Options parsing\n> + */\n> +\n> +#include <getopt.h>\n> +#include <iomanip>\n> +#include <iostream>\n> +#include <string.h>\n> +\n> +#include \"options.h\"\n> +\n> +void OptionsParser::addOption(int opt, const char *help, const char *name,\n> +\t\t\t      OptionArgument argument, const char *argumentName)\n> +{\n> +\t/*\n> +\t * Options must have at least a short or long name, and a text message.\n> +\t * If an argument is accepted, it must be described by argumentName.\n> +\t */\n> +\tif (!isalnum(opt) && !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(opt) != optionsMap_.end())\n> +\t\treturn;\n> +\n> +\toptions_.push_back(Option({ opt, name, argument, argumentName, help }));\n> +\toptionsMap_[opt] = &options_.back();\n> +}\n> +\n> +OptionsParser::Options OptionsParser::parse(int argc, char **argv)\n> +{\n> +\tOptionsParser::Options options;\n> +\n> +\t/*\n> +\t * Allocate short and long options arrays large enough to contain all\n> +\t * options.\n> +\t */\n> +\tchar shortOptions[options_.size() * 3 + 2] = {};\n> +\tstruct option longOptions[options_.size() + 1] = {};\n> +\tunsigned int ids = 0;\n> +\tunsigned int idl = 0;\n> +\n> +\tshortOptions[ids++] = ':';\n> +\n> +\tfor (const Option &option : options_) {\n> +\t\tif (option.hasShortOption()) {\n> +\t\t\tshortOptions[ids++] = option.opt;\n> +\t\t\tif (option.argument != ArgumentNone)\n> +\t\t\t\tshortOptions[ids++] = ':';\n> +\t\t\tif (option.argument == ArgumentOptional)\n> +\t\t\t\tshortOptions[ids++] = ':';\n> +\t\t}\n> +\n> +\t\tif (option.hasLongOption()) {\n> +\t\t\tlongOptions[idl].name = option.name;\n> +\n> +\t\t\tswitch (option.argument) {\n> +\t\t\tcase ArgumentNone:\n> +\t\t\t\tlongOptions[idl].has_arg = no_argument;\n> +\t\t\t\tbreak;\n> +\t\t\tcase ArgumentRequired:\n> +\t\t\t\tlongOptions[idl].has_arg = required_argument;\n> +\t\t\t\tbreak;\n> +\t\t\tcase ArgumentOptional:\n> +\t\t\t\tlongOptions[idl].has_arg = optional_argument;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tlongOptions[idl].flag = 0;\n> +\t\t\tlongOptions[idl].val = option.opt;\n> +\t\t\tidl++;\n> +\t\t}\n> +\t}\n> +\n> +\topterr = 0;\n> +\n> +\twhile (true) {\n> +\t\tint c = getopt_long(argc, argv, shortOptions, longOptions, nullptr);\n> +\n> +\t\tif (c == -1)\n> +\t\t\tbreak;\n> +\n> +\t\tif (c == '?' || c == ':') {\n> +\t\t\tif (c == '?')\n> +\t\t\t\tstd::cerr << \"Invalid option \";\n> +\t\t\telse\n> +\t\t\t\tstd::cerr << \"Missing argument for option \";\n> +\t\t\tstd::cerr << argv[optind - 1] << std::endl;\n> +\n> +\t\t\tusage();\n> +\t\t\toptions.clear();\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\toptions.values_[c] = optarg ? optarg : \"\";\n> +\t}\n> +\n> +\treturn std::move(options);\n> +}\n> +\n> +void OptionsParser::usage()\n> +{\n> +\tstd::cerr << \"Options:\" << std::endl;\n> +\n> +\tunsigned int indent = 0;\n> +\n> +\tfor (const Option &option : options_) {\n> +\t\tunsigned int length = 14;\n> +\t\tif (option.hasLongOption())\n> +\t\t\tlength += 2 + strlen(option.name);\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 > indent)\n> +\t\t\tindent = length;\n> +\t}\n> +\n> +\tindent = (indent + 7) / 8 * 8;\n> +\n> +\tfor (const Option &option : options_) {\n> +\t\tstd::string argument;\n> +\t\tif (option.hasShortOption())\n> +\t\t\targument = std::string(\"  -\")\n> +\t\t\t\t + static_cast<char>(option.opt);\n> +\t\telse\n> +\t\t\targument = \"    \";\n> +\n> +\t\tif (option.hasLongOption()) {\n> +\t\t\tif (option.hasShortOption())\n> +\t\t\t\targument += \", \";\n> +\t\t\telse\n> +\t\t\t\targument += \"  \";\n> +\t\t\targument += std::string(\"--\") + option.name;\n> +\t\t};\n> +\n> +\t\tif (option.argument != ArgumentNone) {\n> +\t\t\targument += std::string(\" \");\n> +\t\t\tif (option.argument == ArgumentOptional)\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::left << argument;\n> +\t\tstd::cerr << option.help << std::endl;\n> +\t}\n> +}\n> +\n> +OptionsParser::Options::Options()\n> +{\n> +}\n> +\n> +OptionsParser::Options::Options(Options &&other)\n> +\t: values_(std::move(other.values_))\n> +{\n> +}\n> +\n> +OptionsParser::Options &OptionsParser::Options::operator=(Options &&other)\n> +{\n> +\tvalues_ = other.values_;\n> +\treturn *this;\n> +}\n> +\n> +bool OptionsParser::Options::valid() const\n> +{\n> +\treturn !values_.empty();\n> +}\n> +\n> +bool OptionsParser::Options::isSet(int opt) const\n> +{\n> +\treturn values_.find(opt) != values_.end();\n> +}\n> +\n> +const std::string &OptionsParser::Options::operator[](int opt) const\n> +{\n> +\treturn values_.find(opt)->second;\n> +}\n> +\n> +void OptionsParser::Options::clear()\n> +{\n> +\tvalues_.clear();\n> +}\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> new file mode 100644\n> index 000000000000..88336dfe3cc6\n> --- /dev/null\n> +++ b/src/cam/options.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * options.h - cam - Options parsing\n> + */\n> +#ifndef __CAM_OPTIONS_H__\n> +#define __CAM_OPTIONS_H__\n> +\n> +#include <ctype.h>\n> +#include <map>\n> +#include <vector>\n> +\n> +class OptionsParser\n> +{\n> +public:\n> +\tenum OptionArgument {\n> +\t\tArgumentNone,\n> +\t\tArgumentRequired,\n> +\t\tArgumentOptional,\n> +\t};\n> +\n> +\tclass Options {\n> +\tpublic:\n> +\t\tOptions();\n> +\t\tOptions(Options &&other);\n> +\t\tOptions &operator=(Options &&other);\n> +\n> +\t\tbool valid() const;\n> +\t\tbool isSet(int opt) const;\n> +\t\tconst std::string &operator[](int opt) const;\n> +\n> +\tprivate:\n> +\t\tfriend class OptionsParser;\n> +\t\tstd::map<int, std::string> values_;\n> +\t\tvoid clear();\n> +\t};\n> +\n> +\tvoid addOption(int opt, const char *help, const char *name = nullptr,\n> +\t\t       OptionArgument argument = ArgumentNone,\n> +\t\t       const char *argumentName = nullptr);\n> +\n> +\tOptions parse(int argc, char *argv[]);\n> +\tvoid usage();\n> +\n> +private:\n> +\tstruct Option {\n> +\t\tint opt;\n> +\t\tconst char *name;\n> +\t\tOptionArgument argument;\n> +\t\tconst char *argumentName;\n> +\t\tconst char *help;\n> +\n> +\t\tbool hasShortOption() const { return isalnum(opt); }\n> +\t\tbool hasLongOption() const { return name != nullptr; }\n> +\t};\n> +\n> +\tstd::vector<Option> options_;\n> +\tstd::map<unsigned int, Option *> optionsMap_;\n> +};\n> +\n> +#endif /* __CAM_OPTIONS_H__ */\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 413C860B21\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 22 Jan 2019 14:07:57 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id u89-v6so20580259lje.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 22 Jan 2019 05:07:57 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\t18-v6sm2850512ljg.83.2019.01.22.05.07.53\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 22 Jan 2019 05:07:54 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=KR2IH8X2Umj8lkuoXg+bsYi8+WrrqYONEZx9RM/yTUw=;\n\tb=P2LRHGQLihfYSkAONRVFbywtYfaaVpBRKV1zhzDNRl6bjgAOTuOB+e4PTUjs7DB26T\n\t1AoK/tecxne59cZYMi+XMuKwr9vjtWe/LU75/Y/hxvf1OY639A4J6c/STDCLCHAFio+1\n\tedWb2z09BoWAFbdvCqqrhk0L8xn6OiU1VM7pwtAHVnjef1krvvIVjfPFmZOarqu+Hmlh\n\t3lt9o1sBaR6SgV0KEuSiloNxv1jV/2R9K5Jd8MQz7yq3JlBUVtPOSRrHroy4UYmf0sg5\n\tfgG18h2lFNf2k9RcSvY3S6Mv+4krPkimxXpqnwWQXvKmx3ZCEPU+HJjdMmApWdYhr/5o\n\tYy6w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=KR2IH8X2Umj8lkuoXg+bsYi8+WrrqYONEZx9RM/yTUw=;\n\tb=gNcaP7VyO5Cc1dy7xeGd1EL5y9tp59+SUGLMFvN3vflUyfobXSA89lQQa6LlWZNliY\n\tInwD5wALVdQGHaTMUNH4rMUCSNURBjlsygFyzxvWC0zU+bO+UXqjmg/KxHsOmbHORsSw\n\tkxooTgfDJlFYFGYYk3kQJPFwLvJMoLKxQAbjFvK601ex5WGJdfu071ypMV+PhHVBH5w4\n\tqAkIWuh5QEeygbpGdWaCw4O8m7UOdPYrmwKt9bgETU/e6WQIDug0ag2WB6uQelnJZzW8\n\twG3fYCcqP3qh5+GuEGg7boHfkE40Mmi9J0ZzuOttK4u0ZzqR4dfeurwdaI336EGUebnd\n\tKavw==","X-Gm-Message-State":"AJcUukfzVJI0Hj+emMcKYOdIy67MyNHLZNZRIARBGvnfHozsp6119XNr\n\tlcGVrtdY1ElGQE5H3FgFFqNfckSBVjQ=","X-Google-Smtp-Source":"ALg8bN7DYwEzLgrnEGtnXSFU3MbD33I4tBIQopeEd15l4kyVehuqnEEsZVG+RX5ki7yOjWDXPQic0A==","X-Received":"by 2002:a2e:2416:: with SMTP id\n\tk22-v6mr22026740ljk.80.1548162474885; \n\tTue, 22 Jan 2019 05:07:54 -0800 (PST)","Date":"Tue, 22 Jan 2019 14:07:53 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122130753.GP6484@bigcity.dyn.berto.se>","References":"<20190121153331.19430-1-niklas.soderlund@ragnatech.se>\n\t<20190122030354.9131-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190122030354.9131-1-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] cam: Extract option parser to\n\tseparate file","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":"Tue, 22 Jan 2019 13:07:57 -0000"}}]