[{"id":17487,"web_url":"https://patchwork.libcamera.org/comment/17487/","msgid":"<YL/xxA2TYUd6tkjq@oden.dyn.berto.se>","date":"2021-06-08T22:40:04","subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Nícolas,\n\nThanks for your work.\n\nOn 2021-06-07 15:15:06 -0300, Nícolas F. R. A. Prado wrote:\n> Add a --list parameter that lists all current tests (by mapping to\n> googletest's --gtest_list_tests).\n> \n> Add a --filter 'filterString' parameter that filters the tests to run\n> (by mapping to googletest's --gtest_filter).\n> \n> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> ---\n> Changes in v6:\n> - Thanks to Niklas:\n>   - Changed buildGtestParameters() into initGtestParameters() and removed the\n>     need for Harness::gtestArgc_ and Harness::gtestArgv_\n> \n> Changes in v5:\n> - Thanks to Niklas:\n>   - Moved buildGtestParameters() inside run()\n> \n> No changes in v4\n> \n>  src/lc-compliance/main.cpp | 69 +++++++++++++++++++++++++++++++++++---\n>  1 file changed, 65 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 27503372d0eb..e35238471ee6 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -20,6 +20,8 @@ using namespace libcamera;\n>  \n>  enum {\n>  \tOptCamera = 'c',\n> +\tOptList = 'l',\n> +\tOptFilter = 'f',\n>  \tOptHelp = 'h',\n>  };\n>  \n> @@ -45,13 +47,17 @@ public:\n>  \t~Harness();\n>  \n>  \tint init();\n> -\tint run(int argc, char **argv);\n> +\tint run(char *arg0);\n>  \n>  private:\n> +\tint initGtestParameters(char *arg0);\n>  \tvoid listCameras();\n>  \n>  \tOptionsParser::Options options_;\n>  \tstd::shared_ptr<CameraManager> cm_;\n> +\n> +\tconst std::map<std::string, std::string> gtestFlag_ = { { \"list\", \"--gtest_list_tests\" },\n> +\t\t\t\t\t\t\t\t{ \"filter\", \"--gtest_filter\" } };\n>  };\n>  \n>  Harness::Harness(const OptionsParser::Options &options)\n> @@ -76,6 +82,12 @@ int Harness::init()\n>  \t\treturn ret;\n>  \t}\n>  \n> +\t/*\n> +\t * No need to initialize the camera if we'll just list tests\n> +\t */\n> +\tif (options_.isSet(OptList))\n> +\t\treturn 0;\n> +\n>  \tif (!options_.isSet(OptCamera)) {\n>  \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n>  \t\tlistCameras();\n> @@ -97,9 +109,11 @@ int Harness::init()\n>  \treturn 0;\n>  }\n>  \n> -int Harness::run(int argc, char **argv)\n> +int Harness::run(char *arg0)\n>  {\n> -\t::testing::InitGoogleTest(&argc, argv);\n> +\tint ret = initGtestParameters(arg0);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n>  \ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n>  \n> @@ -112,12 +126,59 @@ void Harness::listCameras()\n>  \t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n>  }\n>  \n> +int Harness::initGtestParameters(char *arg0)\n> +{\n> +\tint argc = 0;\n> +\tchar **argv;\n> +\tstd::string filterParam;\n> +\n> +\t/*\n> +\t * +2 to have space for both the 0th argument that is needed but not\n> +\t * used and the null at the end.\n> +\t */\n> +\targv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *));\n> +\tif (!argv)\n> +\t\treturn -ENOMEM;\n> +\n> +\targv[argc] = arg0;\n> +\targc++;\n> +\n> +\tif (options_.isSet(OptList)) {\n> +\t\targv[argc] = const_cast<char *>(gtestFlag_.at(\"list\").c_str());\n> +\t\targc++;\n> +\t}\n> +\n> +\tif (options_.isSet(OptFilter)) {\n> +\t\t/*\n> +\t\t * The filter flag needs to be passed as a single parameter, in\n> +\t\t * the format --gtest_filter=filterStr\n> +\t\t */\n> +\t\tconst std::string &filter = options_[OptFilter];\n\nI think 'filter' needs to be declared in the initGtestParameters() scope \nas it's consumed by ::testing::InitGoogleTest(). With this fixed,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nNice work!\n\n> +\t\tfilterParam = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> +\n> +\t\targv[argc] = const_cast<char *>(filterParam.c_str());\n> +\t\targc++;\n> +\t}\n> +\n> +\targv[argc] = 0;\n> +\n> +\t::testing::InitGoogleTest(&argc, argv);\n> +\n> +\tfree(argv);\n> +\n> +\treturn 0;\n> +}\n> +\n>  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n>  {\n>  \tOptionsParser parser;\n>  \tparser.addOption(OptCamera, OptionString,\n>  \t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n>  \t\t\t ArgumentRequired, \"camera\");\n> +\tparser.addOption(OptList, OptionNone, \"List all tests and exit\", \"list\");\n> +\tparser.addOption(OptFilter, OptionString,\n> +\t\t\t \"Specify which tests to run\", \"filter\",\n> +\t\t\t ArgumentRequired, \"filter\");\n>  \tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n>  \t\t\t \"help\");\n>  \n> @@ -147,5 +208,5 @@ int main(int argc, char **argv)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\treturn harness.run(argc, argv);\n> +\treturn harness.run(argv[0]);\n>  }\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B089C3206\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  8 Jun 2021 22:40:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 854ED6892B;\n\tWed,  9 Jun 2021 00:40:08 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63C556891F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  9 Jun 2021 00:40:06 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id s22so8701067ljg.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 08 Jun 2021 15:40:06 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\tv6sm115332lfr.182.2021.06.08.15.40.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 08 Jun 2021 15:40:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Qb1yDdOc\"; dkim-atps=neutral","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\tbh=azOhSGP8IKbFPKH566t5v+syh0gwMG4oFwmF5n7VZUU=;\n\tb=Qb1yDdOcLKqUXpvBlYHmZ3UQgudMg1oQQZzWczpLNbRtdJ9lc5KwvJYDoPWUGd6Mzu\n\tqF8uFc/QiTt4Ovu2ELBiF+XC72ugi1ya9N0cfajnD/u8eBXDjHTqQ2y2GKF/0eLiY3Ma\n\tTNhD9EjkbVSZpi/bdf9KbtQW+y/5MIZg8KH5O9MoDjlKdM3+pyb3/S+R6VQn8qnUp6zb\n\tOmTyatIukpDMgcoIePAhp2qC11nGhTfusu6JfuufqYsN6UVXv+TYoat3/u+iN/tssFKe\n\tQf7lXfLqLXQMxWumMajTKKVDcdgRCxByVcxOEkyr/g3tPUd2KT8kEp6MD4r1NQ60EGrk\n\tguYA==","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;\n\tbh=azOhSGP8IKbFPKH566t5v+syh0gwMG4oFwmF5n7VZUU=;\n\tb=Q+SEBMtpS1ArIEvaQ1LLlbgptWXsnxIwAMYWKrM7J2IO/LFZDvnKnJ7DvWd4mI3hpR\n\tpMX8RjaLg0EuyBezKnyV/+7fyVcETgaEpkYpXlJ6AfHFt13umsm3583vc4mJUyNy6cxY\n\twvPVeX6h3C9A86xPhfWQWG1UkbhqCnfG8R0z+CfymUYzRO0DXi8cN5C0HeRuB3zCxhl9\n\tfF+r0pjXwquX6m/aFwhrCGwdLy8jVrrVzspW65Pu/QA9A2YSLND76DaXGwILCx5j45TN\n\tvAwSfQLvhUvNwBF7rQJjPSvzaep18ckt5HmNruv8igYB1UkMnAGZbSZPw7SSwZLShR3Y\n\tUgIg==","X-Gm-Message-State":"AOAM533tXUwculs6nf2sz94An/n+C+gL6XGJpmRWU8QODPFThpy+Why8\n\tbT1rNBqJkoCbxujcQq+Z50ENhA==","X-Google-Smtp-Source":"ABdhPJwyTAEb4+lb10I6rlCuJKdWrirI8+L/1e4XfSJyfbSNY5VB4B8MD9cbnYVYh8XpvtPVvrqN4g==","X-Received":"by 2002:a05:651c:50c:: with SMTP id\n\to12mr19894850ljp.287.1623192005483; \n\tTue, 08 Jun 2021 15:40:05 -0700 (PDT)","Date":"Wed, 9 Jun 2021 00:40:04 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado <nfraprado@collabora.com>","Message-ID":"<YL/xxA2TYUd6tkjq@oden.dyn.berto.se>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-6-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210607181506.51711-6-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida <andrealmeid@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17491,"web_url":"https://patchwork.libcamera.org/comment/17491/","msgid":"<20210609123040.77jgqmcosmxzce4u@notapiano>","date":"2021-06-09T12:30:40","subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Niklas,\n\nOn Wed, Jun 09, 2021 at 12:40:04AM +0200, Niklas Söderlund wrote:\n> Hi Nícolas,\n> \n> Thanks for your work.\n> \n> On 2021-06-07 15:15:06 -0300, Nícolas F. R. A. Prado wrote:\n> > Add a --list parameter that lists all current tests (by mapping to\n> > googletest's --gtest_list_tests).\n> > \n> > Add a --filter 'filterString' parameter that filters the tests to run\n> > (by mapping to googletest's --gtest_filter).\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> > ---\n> > Changes in v6:\n> > - Thanks to Niklas:\n> >   - Changed buildGtestParameters() into initGtestParameters() and removed the\n> >     need for Harness::gtestArgc_ and Harness::gtestArgv_\n> > \n> > Changes in v5:\n> > - Thanks to Niklas:\n> >   - Moved buildGtestParameters() inside run()\n> > \n> > No changes in v4\n> > \n> >  src/lc-compliance/main.cpp | 69 +++++++++++++++++++++++++++++++++++---\n> >  1 file changed, 65 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 27503372d0eb..e35238471ee6 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -20,6 +20,8 @@ using namespace libcamera;\n> >  \n> >  enum {\n> >  \tOptCamera = 'c',\n> > +\tOptList = 'l',\n> > +\tOptFilter = 'f',\n> >  \tOptHelp = 'h',\n> >  };\n> >  \n> > @@ -45,13 +47,17 @@ public:\n> >  \t~Harness();\n> >  \n> >  \tint init();\n> > -\tint run(int argc, char **argv);\n> > +\tint run(char *arg0);\n> >  \n> >  private:\n> > +\tint initGtestParameters(char *arg0);\n> >  \tvoid listCameras();\n> >  \n> >  \tOptionsParser::Options options_;\n> >  \tstd::shared_ptr<CameraManager> cm_;\n> > +\n> > +\tconst std::map<std::string, std::string> gtestFlag_ = { { \"list\", \"--gtest_list_tests\" },\n> > +\t\t\t\t\t\t\t\t{ \"filter\", \"--gtest_filter\" } };\n> >  };\n> >  \n> >  Harness::Harness(const OptionsParser::Options &options)\n> > @@ -76,6 +82,12 @@ int Harness::init()\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> > +\t/*\n> > +\t * No need to initialize the camera if we'll just list tests\n> > +\t */\n> > +\tif (options_.isSet(OptList))\n> > +\t\treturn 0;\n> > +\n> >  \tif (!options_.isSet(OptCamera)) {\n> >  \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> >  \t\tlistCameras();\n> > @@ -97,9 +109,11 @@ int Harness::init()\n> >  \treturn 0;\n> >  }\n> >  \n> > -int Harness::run(int argc, char **argv)\n> > +int Harness::run(char *arg0)\n> >  {\n> > -\t::testing::InitGoogleTest(&argc, argv);\n> > +\tint ret = initGtestParameters(arg0);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >  \n> >  \ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> >  \n> > @@ -112,12 +126,59 @@ void Harness::listCameras()\n> >  \t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> >  }\n> >  \n> > +int Harness::initGtestParameters(char *arg0)\n> > +{\n> > +\tint argc = 0;\n> > +\tchar **argv;\n> > +\tstd::string filterParam;\n> > +\n> > +\t/*\n> > +\t * +2 to have space for both the 0th argument that is needed but not\n> > +\t * used and the null at the end.\n> > +\t */\n> > +\targv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *));\n> > +\tif (!argv)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\targv[argc] = arg0;\n> > +\targc++;\n> > +\n> > +\tif (options_.isSet(OptList)) {\n> > +\t\targv[argc] = const_cast<char *>(gtestFlag_.at(\"list\").c_str());\n> > +\t\targc++;\n> > +\t}\n> > +\n> > +\tif (options_.isSet(OptFilter)) {\n> > +\t\t/*\n> > +\t\t * The filter flag needs to be passed as a single parameter, in\n> > +\t\t * the format --gtest_filter=filterStr\n> > +\t\t */\n> > +\t\tconst std::string &filter = options_[OptFilter];\n> \n> I think 'filter' needs to be declared in the initGtestParameters() scope \n> as it's consumed by ::testing::InitGoogleTest(). With this fixed,\n\nActually what is consumed in ::testing::InitGoogleTest() is filterParam, which\nindeed is in initGtestParameters()'s scope. This filter string here is used just\nto compose the entire string just below and move it to filterParam. I'm assuming\nwhen I do\n\n\tfilterParam = gtestFlag_.at(\"filter\") + \"=\" + filter;\n\nI'm copying the combined contents of all those strings to filterParam, so it\nshould be safe to use in ::testing::InitGoogleTest(), but please tell me if this\nis not the case :).\n\nBut maybe it would be better to just omit that local filter variable? A cast\nseems to be needed in that case:\n\n\tfilterParam = gtestFlag_.at(\"filter\") + \"=\" +\n\t\t(const std::string&) options_[OptFilter];\n\nWhat do you think?\n\nThanks,\nNícolas\n\n> \n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \n> Nice work!\n> \n> > +\t\tfilterParam = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> > +\n> > +\t\targv[argc] = const_cast<char *>(filterParam.c_str());\n> > +\t\targc++;\n> > +\t}\n> > +\n> > +\targv[argc] = 0;\n> > +\n> > +\t::testing::InitGoogleTest(&argc, argv);\n> > +\n> > +\tfree(argv);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> >  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> >  {\n> >  \tOptionsParser parser;\n> >  \tparser.addOption(OptCamera, OptionString,\n> >  \t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n> >  \t\t\t ArgumentRequired, \"camera\");\n> > +\tparser.addOption(OptList, OptionNone, \"List all tests and exit\", \"list\");\n> > +\tparser.addOption(OptFilter, OptionString,\n> > +\t\t\t \"Specify which tests to run\", \"filter\",\n> > +\t\t\t ArgumentRequired, \"filter\");\n> >  \tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n> >  \t\t\t \"help\");\n> >  \n> > @@ -147,5 +208,5 @@ int main(int argc, char **argv)\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >  \n> > -\treturn harness.run(argc, argv);\n> > +\treturn harness.run(argv[0]);\n> >  }\n> > -- \n> > 2.31.1\n> > \n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3CE51C320B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  9 Jun 2021 12:31:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5302602A0;\n\tWed,  9 Jun 2021 14:31:27 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B07F6029C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  9 Jun 2021 14:31:26 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id 8A07C1F435B1"],"Date":"Wed, 9 Jun 2021 09:30:40 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20210609123040.77jgqmcosmxzce4u@notapiano>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-6-nfraprado@collabora.com>\n\t<YL/xxA2TYUd6tkjq@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<YL/xxA2TYUd6tkjq@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida <andrealmeid@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17510,"web_url":"https://patchwork.libcamera.org/comment/17510/","msgid":"<YMJQU22q6ButCURl@oden.dyn.berto.se>","date":"2021-06-10T17:48:03","subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Nícolas,\n\nOn 2021-06-09 09:30:40 -0300, Nícolas F. R. A. Prado wrote:\n> Hi Niklas,\n> \n> On Wed, Jun 09, 2021 at 12:40:04AM +0200, Niklas Söderlund wrote:\n> > Hi Nícolas,\n> > \n> > Thanks for your work.\n> > \n> > On 2021-06-07 15:15:06 -0300, Nícolas F. R. A. Prado wrote:\n> > > Add a --list parameter that lists all current tests (by mapping to\n> > > googletest's --gtest_list_tests).\n> > > \n> > > Add a --filter 'filterString' parameter that filters the tests to run\n> > > (by mapping to googletest's --gtest_filter).\n> > > \n> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> > > ---\n> > > Changes in v6:\n> > > - Thanks to Niklas:\n> > >   - Changed buildGtestParameters() into initGtestParameters() and removed the\n> > >     need for Harness::gtestArgc_ and Harness::gtestArgv_\n> > > \n> > > Changes in v5:\n> > > - Thanks to Niklas:\n> > >   - Moved buildGtestParameters() inside run()\n> > > \n> > > No changes in v4\n> > > \n> > >  src/lc-compliance/main.cpp | 69 +++++++++++++++++++++++++++++++++++---\n> > >  1 file changed, 65 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > > index 27503372d0eb..e35238471ee6 100644\n> > > --- a/src/lc-compliance/main.cpp\n> > > +++ b/src/lc-compliance/main.cpp\n> > > @@ -20,6 +20,8 @@ using namespace libcamera;\n> > >  \n> > >  enum {\n> > >  \tOptCamera = 'c',\n> > > +\tOptList = 'l',\n> > > +\tOptFilter = 'f',\n> > >  \tOptHelp = 'h',\n> > >  };\n> > >  \n> > > @@ -45,13 +47,17 @@ public:\n> > >  \t~Harness();\n> > >  \n> > >  \tint init();\n> > > -\tint run(int argc, char **argv);\n> > > +\tint run(char *arg0);\n> > >  \n> > >  private:\n> > > +\tint initGtestParameters(char *arg0);\n> > >  \tvoid listCameras();\n> > >  \n> > >  \tOptionsParser::Options options_;\n> > >  \tstd::shared_ptr<CameraManager> cm_;\n> > > +\n> > > +\tconst std::map<std::string, std::string> gtestFlag_ = { { \"list\", \"--gtest_list_tests\" },\n> > > +\t\t\t\t\t\t\t\t{ \"filter\", \"--gtest_filter\" } };\n> > >  };\n> > >  \n> > >  Harness::Harness(const OptionsParser::Options &options)\n> > > @@ -76,6 +82,12 @@ int Harness::init()\n> > >  \t\treturn ret;\n> > >  \t}\n> > >  \n> > > +\t/*\n> > > +\t * No need to initialize the camera if we'll just list tests\n> > > +\t */\n> > > +\tif (options_.isSet(OptList))\n> > > +\t\treturn 0;\n> > > +\n> > >  \tif (!options_.isSet(OptCamera)) {\n> > >  \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> > >  \t\tlistCameras();\n> > > @@ -97,9 +109,11 @@ int Harness::init()\n> > >  \treturn 0;\n> > >  }\n> > >  \n> > > -int Harness::run(int argc, char **argv)\n> > > +int Harness::run(char *arg0)\n> > >  {\n> > > -\t::testing::InitGoogleTest(&argc, argv);\n> > > +\tint ret = initGtestParameters(arg0);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > >  \n> > >  \ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > >  \n> > > @@ -112,12 +126,59 @@ void Harness::listCameras()\n> > >  \t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > >  }\n> > >  \n> > > +int Harness::initGtestParameters(char *arg0)\n> > > +{\n> > > +\tint argc = 0;\n> > > +\tchar **argv;\n> > > +\tstd::string filterParam;\n> > > +\n> > > +\t/*\n> > > +\t * +2 to have space for both the 0th argument that is needed but not\n> > > +\t * used and the null at the end.\n> > > +\t */\n> > > +\targv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *));\n> > > +\tif (!argv)\n> > > +\t\treturn -ENOMEM;\n> > > +\n> > > +\targv[argc] = arg0;\n> > > +\targc++;\n> > > +\n> > > +\tif (options_.isSet(OptList)) {\n> > > +\t\targv[argc] = const_cast<char *>(gtestFlag_.at(\"list\").c_str());\n> > > +\t\targc++;\n> > > +\t}\n> > > +\n> > > +\tif (options_.isSet(OptFilter)) {\n> > > +\t\t/*\n> > > +\t\t * The filter flag needs to be passed as a single parameter, in\n> > > +\t\t * the format --gtest_filter=filterStr\n> > > +\t\t */\n> > > +\t\tconst std::string &filter = options_[OptFilter];\n> > \n> > I think 'filter' needs to be declared in the initGtestParameters() scope \n> > as it's consumed by ::testing::InitGoogleTest(). With this fixed,\n> \n> Actually what is consumed in ::testing::InitGoogleTest() is filterParam, which\n> indeed is in initGtestParameters()'s scope. This filter string here is used just\n> to compose the entire string just below and move it to filterParam. I'm assuming\n> when I do\n\nYou are correct.\n\n> \n> \tfilterParam = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> \n> I'm copying the combined contents of all those strings to filterParam, so it\n> should be safe to use in ::testing::InitGoogleTest(), but please tell me if this\n> is not the case :).\n> \n> But maybe it would be better to just omit that local filter variable? A cast\n> seems to be needed in that case:\n> \n> \tfilterParam = gtestFlag_.at(\"filter\") + \"=\" +\n> \t\t(const std::string&) options_[OptFilter];\n> \n> What do you think?\n\nAt least it would avoid me making a similar error again while reading \nthe code :-) I think both solutions are ok.\n\n> \n> Thanks,\n> Nícolas\n> \n> > \n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > \n> > Nice work!\n> > \n> > > +\t\tfilterParam = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> > > +\n> > > +\t\targv[argc] = const_cast<char *>(filterParam.c_str());\n> > > +\t\targc++;\n> > > +\t}\n> > > +\n> > > +\targv[argc] = 0;\n> > > +\n> > > +\t::testing::InitGoogleTest(&argc, argv);\n> > > +\n> > > +\tfree(argv);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > >  static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > >  {\n> > >  \tOptionsParser parser;\n> > >  \tparser.addOption(OptCamera, OptionString,\n> > >  \t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n> > >  \t\t\t ArgumentRequired, \"camera\");\n> > > +\tparser.addOption(OptList, OptionNone, \"List all tests and exit\", \"list\");\n> > > +\tparser.addOption(OptFilter, OptionString,\n> > > +\t\t\t \"Specify which tests to run\", \"filter\",\n> > > +\t\t\t ArgumentRequired, \"filter\");\n> > >  \tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n> > >  \t\t\t \"help\");\n> > >  \n> > > @@ -147,5 +208,5 @@ int main(int argc, char **argv)\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >  \n> > > -\treturn harness.run(argc, argv);\n> > > +\treturn harness.run(argv[0]);\n> > >  }\n> > > -- \n> > > 2.31.1\n> > > \n> > \n> > -- \n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3F79BD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 10 Jun 2021 17:48:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 438D96029C;\n\tThu, 10 Jun 2021 19:48:08 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4306F6029B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 10 Jun 2021 19:48:06 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id j2so4524752lfg.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 10 Jun 2021 10:48:06 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\ta14sm416505ljj.86.2021.06.10.10.48.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Jun 2021 10:48:04 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"BVMgvH2a\"; dkim-atps=neutral","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\tbh=loQX7oQDN+cpksDQniHL5Ki6B1SlmhcEAwors5VWjPQ=;\n\tb=BVMgvH2axp1L9qzHyDapSqyy2JgQXlgidCnvbYqtv4nbJQpIH4HI1igAMxuBeDcSCx\n\t2N/+bjS/unMyl9c4kHplbb17S2b9ZohYdYWFcEuLSGxiNgHmh0vPkCLFGvxvBJJ1oG94\n\teXCwagxA41RdB7FcaANwbDSZePHtnKbYiqveeenJYwCmwFBFsbyiuIHLIg0cjB3bf7wJ\n\tU9uF385/cxARitpFzBOMuSDzlv8nu7QGXFwI5UhoK4GNKqzGBgYVmUnuBUsqxBoN6jGC\n\txVB2ipJqqYOOx14yq5f0BnsTMDDcj3/XwlhG/xkaQNT6+Iwn1ada+lCa2daDA+eKjZTn\n\t4APg==","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;\n\tbh=loQX7oQDN+cpksDQniHL5Ki6B1SlmhcEAwors5VWjPQ=;\n\tb=OsAobYOI6EfrUqP/o8W2SFEaBmMbEOqxyBoqHRRhksgy+P4+KZ+4Q9p8R2NdD0XbkP\n\toEuHH7Ac5FXa9rOJNyf8E31fFd+beb6pYv9he7HRK1cJslrV+zm5SJ1oscblrwerpy6K\n\tyW42FhLeMIYiCwQ61SI7SkhT5vu5PkEpdzn3wvzHJNWvbq1A8c4ohR/QE9zi4n1jRbPF\n\tjVGlins/Q/K0T+2stmUxGt8/Svd0IVGKdXbyOo+0zy8s59ViEWaIez4E1PgpZ6WxDg4c\n\t4U0C1/+ovKEBAJyV9T8DTmvF1fg6UsroLo+i1ZToTMx49lfC65wvm70Sp/5pheAKxNn9\n\tBeBA==","X-Gm-Message-State":"AOAM5327catVr7hW7YEWtnxqw+EdxTkiz5My/qbtjc+g+iiBqnuu9FS7\n\tvtQnSSGZ76IO90xOe5HT/F3IVQ==","X-Google-Smtp-Source":"ABdhPJxwaEmJizrra4Jq7E/Xi9MMBmx0m6347S0dVDNOcEoh6Jun94uHuBaEoyAWcDQUpUXe64UZwA==","X-Received":"by 2002:a05:6512:12c9:: with SMTP id\n\tp9mr32057lfg.74.1623347285420; \n\tThu, 10 Jun 2021 10:48:05 -0700 (PDT)","Date":"Thu, 10 Jun 2021 19:48:03 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado <nfraprado@collabora.com>","Message-ID":"<YMJQU22q6ButCURl@oden.dyn.berto.se>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-6-nfraprado@collabora.com>\n\t<YL/xxA2TYUd6tkjq@oden.dyn.berto.se>\n\t<20210609123040.77jgqmcosmxzce4u@notapiano>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210609123040.77jgqmcosmxzce4u@notapiano>","Subject":"Re: [libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and\n\tfilter parameters","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida <andrealmeid@collabora.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]