[{"id":3621,"web_url":"https://patchwork.libcamera.org/comment/3621/","msgid":"<20200206193627.GC7611@pendragon.ideasonboard.com>","date":"2020-02-06T19:36:27","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Feb 05, 2020 at 01:04:18PM +0000, Kieran Bingham wrote:\n> Provide an optional means to recurse into subdirectories to search for IPA\n\ns/means/mean/\n\n> libraries. This allows IPAs contained within their own build directory\n> to be resolved when loading from a non-installed build.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/include/ipa_manager.h | 6 ++--\n> src/libcamera/ipa_manager.cpp | 54 ++++++++++++++++++++++++-----\n> 2 files changed, 49 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h\n> index 94d35d9062e4..8243ba5a1f51 100644\n> --- a/src/libcamera/include/ipa_manager.h\n> +++ b/src/libcamera/include/ipa_manager.h\n> @@ -32,8 +32,10 @@ private:\n> \tIPAManager();\n> \t~IPAManager();\n> \n> -\tint addDir(const char *libDir);\n> -\tint addPath(const char *path);\n> +\tint parseDir(const char *libDir, std::vector &paths,\n> +\t\t unsigned int subdirs);\n> +\tint addDir(const char *libDir, unsigned int subdirs = 0);\n> +\tint addPath(const char *path, unsigned int subdirs = 0);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 048465c37772..24fe709108fe 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -140,16 +140,17 @@ IPAManager *IPAManager::instance()\n> }\n> \n> /**\n> - * \\brief Load IPA modules from a directory\n> + * \\brief Identify shared library objects within a directory\n\nAs the function finds shared libraries, maybe s/Identify/Find/ ? I would\nalso rename the function to findSharedObjects() or something similar.\n\n> * \\param[in] libDir directory to search for IPA modules\n\nWhile at it could you s/directory/Directory/ ?\n\nMissing documentation for the paths parameters, which I would rename to\nfiles.\n\n> + * \\param[in] subdirs maximum number of sub-directories to parse\n\nSame here.\n\ns/subdirs/maxDepth/ ?\n\nDo we need to specify a maximum depth ? The only place where you parse\ndirectories recursively passes true as the subdirs value. It should pass\n1 instead, but that seems a bit arbitrary.\n\n> *\n> - * This method tries to create an IPAModule instance for every shared object\n> - * found in \\a libDir, and skips invalid IPA modules.\n> + * Search a directory for .so files. allowing recursion down to\n\nDid you mean s/files\\./files,/ ?\n\n> + * subdirectories no further than the quantity specified by \\a subdirs\n\ns/quantity/levels/ ?\ns/$/./\n\nYou should add a sentence to state that the found shared objects are\nadded to the files vector.\n\n> *\n> - * \\return Number of modules loaded by this call, or a negative error code\n> - * otherwise\n> + * \\return 0 on success or a negative error code otherwise\n> */\n> -int IPAManager::addDir(const char *libDir)\n> +int IPAManager::parseDir(const char *libDir, std::vector &paths,\n> +\t\t\t unsigned int subdirs)\n> {\n> \tstruct dirent *ent;\n> \tDIR *dir;\n> @@ -158,8 +159,20 @@ int IPAManager::addDir(const char *libDir)\n> \tif (!dir)\n> \t\treturn -errno;\n> \n> -\tstd::vector paths;\n> \twhile ((ent = readdir(dir)) != nullptr) {\n> +\t\tif (ent->d_type == DT_DIR && subdirs) {\n> +\t\t\tif (strcmp(ent->d_name, \".\") == 0 ||\n> +\t\t\t strcmp(ent->d_name, \"..\") == 0)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstd::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> +\n> +\t\t\t/* Only recurse to the limit specified by subdirs */\n\ns/subdirs/subdirs./\n\n> +\t\t\tparseDir(subdir.c_str(), paths, subdirs - 1);\n> +\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\tint offset = strlen(ent->d_name) - 3;\n> \t\tif (offset < 0)\n> \t\t\tcontinue;\n> @@ -170,6 +183,28 @@ int IPAManager::addDir(const char *libDir)\n> \t}\n> \tclosedir(dir);\n> \n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Load IPA modules from a directory\n> + * \\param[in] libDir directory to search for IPA modules\n> + * \\param[in] subdirs maximum number of sub-directories to parse\n\ns/maximum/Maximum/\n\nIt's the maximum depth level, not the maximum number of directories.\nSame comments as for parseDir().\n\n> + *\n> + * This method tries to create an IPAModule instance for every shared object\n> + * found in \\a libDir, and skips invalid IPA modules.\n\nHere too, shouldn't you add a sentence to explain subdirs ?\n\n> + *\n> + * \\return Number of modules loaded by this call, or a negative error code\n> + * otherwise\n> + */\n> +int IPAManager::addDir(const char *libDir, unsigned int subdirs)\n> +{\n> +\tstd::vector paths;\n> +\n> +\tint ret = parseDir(libDir, paths, subdirs);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> \t/* Ensure a stable ordering of modules. */\n> \tstd::sort(paths.begin(), paths.end());\n> \n> @@ -193,6 +228,7 @@ int IPAManager::addDir(const char *libDir)\n> /**\n> * \\brief Load IPA modules from a colon separated PATH variable\n> * \\param[in] path string to split to search for IPA modules\n> + * \\param[in] subdirs maximum number of sub-directories to parse\n\nDitto.\n\n> *\n> * This method tries to create an IPAModule instance for every shared object\n> * found in the directories described by \\a paths.\n\nSame.\n\n> @@ -200,7 +236,7 @@ int IPAManager::addDir(const char *libDir)\n> * \\return Number of modules loaded by this call, or a negative error code\n> * otherwise\n> */\n> -int IPAManager::addPath(const char *paths)\n> +int IPAManager::addPath(const char *paths, unsigned int subdirs)\n> {\n> \tint ipaCount = 0;\n> \n> @@ -210,7 +246,7 @@ int IPAManager::addPath(const char *paths)\n> \n> \t\tif (count) {\n> \t\t\tstd::string path(paths, count);\n> -\t\t\tint ret = addDir(path.c_str());\n> +\t\t\tint ret = addDir(path.c_str(), subdirs);\n> \t\t\tif (ret > 0)\n> \t\t\t\tipaCount += ret;\n> \t\t}","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 94AF560864\n\tfor ;\n\tThu, 6 Feb 2020 20:36:43 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E0C09F0;\n\tThu, 6 Feb 2020 20:36:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581017803;\n\tbh=tee119wZOKqcRki/FzKKRt04PPwUDQiA61Wm8DnUNcU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gRvDVraimqb/br2RoFia/2mboux4bO/hHyuo/g3LBZYklSpXFCirYM3tUoWngyZAj\n\tvE41gSukRPPoilMlIUoCNCr801vmKuc/bgSKjEwoBlyKpcpua+3YD+kTX+A59OU0ju\n\tegy1yLvQoWZzWRLzciw5xVwho+EyCBSrBTWgRzsM=","Date":"Thu, 6 Feb 2020 21:36:27 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200206193627.GC7611@pendragon.ideasonboard.com>","References":"<20200205130420.8736-1-kieran.bingham@ideasonboard.com>\n\t<20200205130420.8736-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200205130420.8736-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Feb 2020 19:36:43 -0000"}},{"id":3743,"web_url":"https://patchwork.libcamera.org/comment/3743/","msgid":"<265fef88-48e7-a6e0-3f52-348df6f1b9ae@ideasonboard.com>","date":"2020-02-13T17:55:46","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/02/2020 19:36, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Feb 05, 2020 at 01:04:18PM +0000, Kieran Bingham wrote:\n>> Provide an optional means to recurse into subdirectories to search for IPA\n> \n> s/means/mean/\n\nUhhh ? I don't think so...\n\nMeans to an end...\nnot\nMean to an end.\n\n\n> \n>> libraries. This allows IPAs contained within their own build directory\n>> to be resolved when loading from a non-installed build.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/include/ipa_manager.h | 6 ++--\n>> src/libcamera/ipa_manager.cpp | 54 ++++++++++++++++++++++++-----\n>> 2 files changed, 49 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h\n>> index 94d35d9062e4..8243ba5a1f51 100644\n>> --- a/src/libcamera/include/ipa_manager.h\n>> +++ b/src/libcamera/include/ipa_manager.h\n>> @@ -32,8 +32,10 @@ private:\n>> \tIPAManager();\n>> \t~IPAManager();\n>> \n>> -\tint addDir(const char *libDir);\n>> -\tint addPath(const char *path);\n>> +\tint parseDir(const char *libDir, std::vector &paths,\n>> +\t\t unsigned int subdirs);\n>> +\tint addDir(const char *libDir, unsigned int subdirs = 0);\n>> +\tint addPath(const char *path, unsigned int subdirs = 0);\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n>> index 048465c37772..24fe709108fe 100644\n>> --- a/src/libcamera/ipa_manager.cpp\n>> +++ b/src/libcamera/ipa_manager.cpp\n>> @@ -140,16 +140,17 @@ IPAManager *IPAManager::instance()\n>> }\n>> \n>> /**\n>> - * \\brief Load IPA modules from a directory\n>> + * \\brief Identify shared library objects within a directory\n> \n> As the function finds shared libraries, maybe s/Identify/Find/ ? I would\n> also rename the function to findSharedObjects() or something similar.\n\nHrm ... Identifying and finding are quite similar, perhaps it's just a\ndistinction of classifying the file, and iterating to be able to\nclassify it...\n\n> \n>> * \\param[in] libDir directory to search for IPA modules\n> \n> While at it could you s/directory/Directory/ ?\n> \n> Missing documentation for the paths parameters, which I would rename to\n> files.\n\nfiles or libraries ?\n\n\nI.e. should we have:\n\n\tfor (const std::string &file : files) {\n\t\tIPAModule *ipaModule = new IPAModule(file);\n\nor\n\n\tfor (const std::string &library : libraries) {\n\t\tIPAModule *ipaModule = new IPAModule(library);\n\n\n> \n>> + * \\param[in] subdirs maximum number of sub-directories to parse\n> \n> Same here.\n> \n> s/subdirs/maxDepth/ ?\n\nI certainly prefer maxDepth. 'subdirs' is a left over from when it was a\nbool.\n\n> Do we need to specify a maximum depth ? The only place where you parse\n> directories recursively passes true as the subdirs value. It should pass\n> 1 instead, but that seems a bit arbitrary.\n\nAha - then it should pass 1 instead :-)\n\nI started with it as a bool, but realised that effectively it's the\ndepth, and if we want to go deeper, then we can set it to 2... or 3...\n\nThat could happen if an IPA creates subdirectories for some structure or\nsuch, or we need to start higher up to search.\n\nAnd I wanted to ensure there was a limit to prevent it going too far if\nthat ever happened.\n\n\n>> *\n>> - * This method tries to create an IPAModule instance for every shared object\n>> - * found in \\a libDir, and skips invalid IPA modules.\n>> + * Search a directory for .so files. allowing recursion down to\n> \n> Did you mean s/files\\./files,/ ?\n\nYup.\n\n> \n>> + * subdirectories no further than the quantity specified by \\a subdirs\n> \n> s/quantity/levels/ ?\n> s/$/./\n\nHrm ... I'm not sure about 'levels'. depth (given maxDepth)?\n\n> \n> You should add a sentence to state that the found shared objects are\n> added to the files vector.\n> \n\n * Discovered shared objects are added to the files vector.\n\n(unless we s/files/libraries/ of course)\n\n>> *\n>> - * \\return Number of modules loaded by this call, or a negative error code\n>> - * otherwise\n>> + * \\return 0 on success or a negative error code otherwise\n>> */\n>> -int IPAManager::addDir(const char *libDir)\n>> +int IPAManager::parseDir(const char *libDir, std::vector &paths,\n>> +\t\t\t unsigned int subdirs)\n>> {\n>> \tstruct dirent *ent;\n>> \tDIR *dir;\n>> @@ -158,8 +159,20 @@ int IPAManager::addDir(const char *libDir)\n>> \tif (!dir)\n>> \t\treturn -errno;\n>> \n>> -\tstd::vector paths;\n>> \twhile ((ent = readdir(dir)) != nullptr) {\n>> +\t\tif (ent->d_type == DT_DIR && subdirs) {\n>> +\t\t\tif (strcmp(ent->d_name, \".\") == 0 ||\n>> +\t\t\t strcmp(ent->d_name, \"..\") == 0)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tstd::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n>> +\n>> +\t\t\t/* Only recurse to the limit specified by subdirs */\n> \n> s/subdirs/subdirs./\n\n\nI think you should codify the rules for periods into checkstyle.py :-)\nOr alternatively write them down in the documentation somewhere.\n\n(I wouldn't ordinarily have used a period for a single line comment)\n\nHowever, in this instance with s/subdirs/maxDepth/ - the code is self\ndocumenting, and I will remove that comment anyway.\n\n\n\n>> +\t\t\tparseDir(subdir.c_str(), paths, subdirs - 1);\n>> +\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> \t\tint offset = strlen(ent->d_name) - 3;\n>> \t\tif (offset < 0)\n>> \t\t\tcontinue;\n>> @@ -170,6 +183,28 @@ int IPAManager::addDir(const char *libDir)\n>> \t}\n>> \tclosedir(dir);\n>> \n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Load IPA modules from a directory\n>> + * \\param[in] libDir directory to search for IPA modules\n>> + * \\param[in] subdirs maximum number of sub-directories to parse\n> \n> s/maximum/Maximum/\n> \n> It's the maximum depth level, not the maximum number of directories.\n> Same comments as for parseDir().\n\n\nagreed it's a max depth.\n\n>> + *\n>> + * This method tries to create an IPAModule instance for every shared object\n>> + * found in \\a libDir, and skips invalid IPA modules.\n> \n> Here too, shouldn't you add a sentence to explain subdirs ?\n> \n>> + *\n>> + * \\return Number of modules loaded by this call, or a negative error code\n>> + * otherwise\n>> + */\n>> +int IPAManager::addDir(const char *libDir, unsigned int subdirs)\n>> +{\n>> +\tstd::vector paths;\n>> +\n>> +\tint ret = parseDir(libDir, paths, subdirs);\n>> +\tif (ret < 0)\n>> +\t\treturn ret;\n>> +\n>> \t/* Ensure a stable ordering of modules. */\n>> \tstd::sort(paths.begin(), paths.end());\n>> \n>> @@ -193,6 +228,7 @@ int IPAManager::addDir(const char *libDir)\n>> /**\n>> * \\brief Load IPA modules from a colon separated PATH variable\n>> * \\param[in] path string to split to search for IPA modules\n>> + * \\param[in] subdirs maximum number of sub-directories to parse\n> \n> Ditto.\n> \n>> *\n>> * This method tries to create an IPAModule instance for every shared object\n>> * found in the directories described by \\a paths.\n> \n> Same.\n> \n>> @@ -200,7 +236,7 @@ int IPAManager::addDir(const char *libDir)\n>> * \\return Number of modules loaded by this call, or a negative error code\n>> * otherwise\n>> */\n>> -int IPAManager::addPath(const char *paths)\n>> +int IPAManager::addPath(const char *paths, unsigned int subdirs)\n>> {\n>> \tint ipaCount = 0;\n>> \n>> @@ -210,7 +246,7 @@ int IPAManager::addPath(const char *paths)\n>> \n>> \t\tif (count) {\n>> \t\t\tstd::string path(paths, count);\n>> -\t\t\tint ret = addDir(path.c_str());\n>> +\t\t\tint ret = addDir(path.c_str(), subdirs);\n>> \t\t\tif (ret > 0)\n>> \t\t\t\tipaCount += ret;\n>> \t\t}\n>","headers":{"Return-Path":"","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 35DB26195F\n\tfor ;\n\tThu, 13 Feb 2020 18:55:50 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90E29504;\n\tThu, 13 Feb 2020 18:55:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581616549;\n\tbh=2O/58DsQw4l9Lr9rLeE8xeCftShjoHqD3vx9FtkRnkY=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XeVPMxA2RTspgBCNAVDbaixTasT7lsWupLnTN1fE8JVGVQygWQ18esX9sgZiGH2ip\n\tbWCBZwrU5ZI5iLVHbtT5AR509Pr8KvYO9gDcTkW+mcsRZKvK4tL6ycRyjZIEGutHps\n\tPLHOaSHeO0IB6bN7epOhGMuL0p9DkfcM7UNywMAE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20200205130420.8736-1-kieran.bingham@ideasonboard.com>\n\t<20200205130420.8736-4-kieran.bingham@ideasonboard.com>\n\t<20200206193627.GC7611@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<265fef88-48e7-a6e0-3f52-348df6f1b9ae@ideasonboard.com>","Date":"Thu, 13 Feb 2020 17:55:46 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200206193627.GC7611@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 13 Feb 2020 17:55:50 -0000"}},{"id":3744,"web_url":"https://patchwork.libcamera.org/comment/3744/","msgid":"<20200213181916.GL29760@pendragon.ideasonboard.com>","date":"2020-02-13T18:19:16","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Feb 13, 2020 at 05:55:46PM +0000, Kieran Bingham wrote:\n> On 06/02/2020 19:36, Laurent Pinchart wrote:\n> > On Wed, Feb 05, 2020 at 01:04:18PM +0000, Kieran Bingham wrote:\n> >> Provide an optional means to recurse into subdirectories to search for IPA\n> > \n> > s/means/mean/\n> \n> Uhhh ? I don't think so...\n> \n> Means to an end...\n> not\n> Mean to an end.\n\nInteresting, https://en.wiktionary.org/wiki/means\n\nmeans\n\n plural of mean\n\nmeans (plural means)\n\n An instrument or condition for attaining a purpose.\n\nSo means is both the plural of mean, but also a singular whose plural is\nmeans. And people say that French is difficult :-)\n\n> >> libraries. This allows IPAs contained within their own build directory\n> >> to be resolved when loading from a non-installed build.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/include/ipa_manager.h | 6 ++--\n> >> src/libcamera/ipa_manager.cpp | 54 ++++++++++++++++++++++++-----\n> >> 2 files changed, 49 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h\n> >> index 94d35d9062e4..8243ba5a1f51 100644\n> >> --- a/src/libcamera/include/ipa_manager.h\n> >> +++ b/src/libcamera/include/ipa_manager.h\n> >> @@ -32,8 +32,10 @@ private:\n> >> \tIPAManager();\n> >> \t~IPAManager();\n> >> \n> >> -\tint addDir(const char *libDir);\n> >> -\tint addPath(const char *path);\n> >> +\tint parseDir(const char *libDir, std::vector &paths,\n> >> +\t\t unsigned int subdirs);\n> >> +\tint addDir(const char *libDir, unsigned int subdirs = 0);\n> >> +\tint addPath(const char *path, unsigned int subdirs = 0);\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> >> index 048465c37772..24fe709108fe 100644\n> >> --- a/src/libcamera/ipa_manager.cpp\n> >> +++ b/src/libcamera/ipa_manager.cpp\n> >> @@ -140,16 +140,17 @@ IPAManager *IPAManager::instance()\n> >> }\n> >> \n> >> /**\n> >> - * \\brief Load IPA modules from a directory\n> >> + * \\brief Identify shared library objects within a directory\n> > \n> > As the function finds shared libraries, maybe s/Identify/Find/ ? I would\n> > also rename the function to findSharedObjects() or something similar.\n> \n> Hrm ... Identifying and finding are quite similar, perhaps it's just a\n> distinction of classifying the file, and iterating to be able to\n> classify it...\n> \n> >> * \\param[in] libDir directory to search for IPA modules\n> > \n> > While at it could you s/directory/Directory/ ?\n> > \n> > Missing documentation for the paths parameters, which I would rename to\n> > files.\n> \n> files or libraries ?\n> \n> I.e. should we have:\n> \n> \tfor (const std::string &file : files) {\n> \t\tIPAModule *ipaModule = new IPAModule(file);\n> \n> or\n> \n> \tfor (const std::string &library : libraries) {\n> \t\tIPAModule *ipaModule = new IPAModule(library);\n\nI'm fine with either.\n\n> > \n> >> + * \\param[in] subdirs maximum number of sub-directories to parse\n> > \n> > Same here.\n> > \n> > s/subdirs/maxDepth/ ?\n> \n> I certainly prefer maxDepth. 'subdirs' is a left over from when it was a\n> bool.\n> \n> > Do we need to specify a maximum depth ? The only place where you parse\n> > directories recursively passes true as the subdirs value. It should pass\n> > 1 instead, but that seems a bit arbitrary.\n> \n> Aha - then it should pass 1 instead :-)\n> \n> I started with it as a bool, but realised that effectively it's the\n> depth, and if we want to go deeper, then we can set it to 2... or 3...\n> \n> That could happen if an IPA creates subdirectories for some structure or\n> such, or we need to start higher up to search.\n> \n> And I wanted to ensure there was a limit to prevent it going too far if\n> that ever happened.\n> \n> >> *\n> >> - * This method tries to create an IPAModule instance for every shared object\n> >> - * found in \\a libDir, and skips invalid IPA modules.\n> >> + * Search a directory for .so files. allowing recursion down to\n> > \n> > Did you mean s/files\\./files,/ ?\n> \n> Yup.\n> \n> > \n> >> + * subdirectories no further than the quantity specified by \\a subdirs\n> > \n> > s/quantity/levels/ ?\n> > s/$/./\n> \n> Hrm ... I'm not sure about 'levels'. depth (given maxDepth)?\n\nWorks for me.\n\n> > You should add a sentence to state that the found shared objects are\n> > added to the files vector.\n> \n> * Discovered shared objects are added to the files vector.\n> \n> (unless we s/files/libraries/ of course)\n> \n> >> *\n> >> - * \\return Number of modules loaded by this call, or a negative error code\n> >> - * otherwise\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> */\n> >> -int IPAManager::addDir(const char *libDir)\n> >> +int IPAManager::parseDir(const char *libDir, std::vector &paths,\n> >> +\t\t\t unsigned int subdirs)\n> >> {\n> >> \tstruct dirent *ent;\n> >> \tDIR *dir;\n> >> @@ -158,8 +159,20 @@ int IPAManager::addDir(const char *libDir)\n> >> \tif (!dir)\n> >> \t\treturn -errno;\n> >> \n> >> -\tstd::vector paths;\n> >> \twhile ((ent = readdir(dir)) != nullptr) {\n> >> +\t\tif (ent->d_type == DT_DIR && subdirs) {\n> >> +\t\t\tif (strcmp(ent->d_name, \".\") == 0 ||\n> >> +\t\t\t strcmp(ent->d_name, \"..\") == 0)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\tstd::string subdir = std::string(libDir) + \"/\" + ent->d_name;\n> >> +\n> >> +\t\t\t/* Only recurse to the limit specified by subdirs */\n> > \n> > s/subdirs/subdirs./\n> \n> I think you should codify the rules for periods into checkstyle.py :-)\n\nI tried to, but it involves parsing comments, and was more complex than\nI thought it would be. Implementing a C++ parser in checkstyle.py is\nlikely overkill. It would be nice if we could extend clang-format with\nadditional rules written in python :-)\n\n> Or alternatively write them down in the documentation somewhere.\n> \n> (I wouldn't ordinarily have used a period for a single line comment)\n> \n> However, in this instance with s/subdirs/maxDepth/ - the code is self\n> documenting, and I will remove that comment anyway.\n> \n> >> +\t\t\tparseDir(subdir.c_str(), paths, subdirs - 1);\n> >> +\n> >> +\t\t\tcontinue;\n> >> +\t\t}\n> >> +\n> >> \t\tint offset = strlen(ent->d_name) - 3;\n> >> \t\tif (offset < 0)\n> >> \t\t\tcontinue;\n> >> @@ -170,6 +183,28 @@ int IPAManager::addDir(const char *libDir)\n> >> \t}\n> >> \tclosedir(dir);\n> >> \n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Load IPA modules from a directory\n> >> + * \\param[in] libDir directory to search for IPA modules\n> >> + * \\param[in] subdirs maximum number of sub-directories to parse\n> > \n> > s/maximum/Maximum/\n> > \n> > It's the maximum depth level, not the maximum number of directories.\n> > Same comments as for parseDir().\n> \n> agreed it's a max depth.\n> \n> >> + *\n> >> + * This method tries to create an IPAModule instance for every shared object\n> >> + * found in \\a libDir, and skips invalid IPA modules.\n> > \n> > Here too, shouldn't you add a sentence to explain subdirs ?\n> > \n> >> + *\n> >> + * \\return Number of modules loaded by this call, or a negative error code\n> >> + * otherwise\n> >> + */\n> >> +int IPAManager::addDir(const char *libDir, unsigned int subdirs)\n> >> +{\n> >> +\tstd::vector paths;\n> >> +\n> >> +\tint ret = parseDir(libDir, paths, subdirs);\n> >> +\tif (ret < 0)\n> >> +\t\treturn ret;\n> >> +\n> >> \t/* Ensure a stable ordering of modules. */\n> >> \tstd::sort(paths.begin(), paths.end());\n> >> \n> >> @@ -193,6 +228,7 @@ int IPAManager::addDir(const char *libDir)\n> >> /**\n> >> * \\brief Load IPA modules from a colon separated PATH variable\n> >> * \\param[in] path string to split to search for IPA modules\n> >> + * \\param[in] subdirs maximum number of sub-directories to parse\n> > \n> > Ditto.\n> > \n> >> *\n> >> * This method tries to create an IPAModule instance for every shared object\n> >> * found in the directories described by \\a paths.\n> > \n> > Same.\n> > \n> >> @@ -200,7 +236,7 @@ int IPAManager::addDir(const char *libDir)\n> >> * \\return Number of modules loaded by this call, or a negative error code\n> >> * otherwise\n> >> */\n> >> -int IPAManager::addPath(const char *paths)\n> >> +int IPAManager::addPath(const char *paths, unsigned int subdirs)\n> >> {\n> >> \tint ipaCount = 0;\n> >> \n> >> @@ -210,7 +246,7 @@ int IPAManager::addPath(const char *paths)\n> >> \n> >> \t\tif (count) {\n> >> \t\t\tstd::string path(paths, count);\n> >> -\t\t\tint ret = addDir(path.c_str());\n> >> +\t\t\tint ret = addDir(path.c_str(), subdirs);\n> >> \t\t\tif (ret > 0)\n> >> \t\t\t\tipaCount += ret;\n> >> \t\t}","headers":{"Return-Path":"","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 4405D6195F\n\tfor ;\n\tThu, 13 Feb 2020 19:19:34 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1DEB504;\n\tThu, 13 Feb 2020 19:19:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581617973;\n\tbh=LLLLJnrnbYVhtQRoe9UMHXt+iM2K0nAQUJ2pqFg6vU8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UGORQ3Vz70YwbZ7dyOz76RF0GA45zh/wdIZsX2A9HQkhOkcFd3dGAysXG+DGjZpiI\n\tAW9Nul5k+8Yk7cCxscfUwm1/Lkzi5eV+5X7Pi8rp29uTN8XkWSaC+lcofvW/ZX8uNj\n\tkgdKyhgGdwzDqw3lCP9/e18wGN8KFyAp3VUiMYAI=","Date":"Thu, 13 Feb 2020 20:19:16 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20200213181916.GL29760@pendragon.ideasonboard.com>","References":"<20200205130420.8736-1-kieran.bingham@ideasonboard.com>\n\t<20200205130420.8736-4-kieran.bingham@ideasonboard.com>\n\t<20200206193627.GC7611@pendragon.ideasonboard.com>\n\t<265fef88-48e7-a6e0-3f52-348df6f1b9ae@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<265fef88-48e7-a6e0-3f52-348df6f1b9ae@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow\n\trecursive parsing","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 13 Feb 2020 18:19:34 -0000"}}]