[{"id":3822,"web_url":"https://patchwork.libcamera.org/comment/3822/","msgid":"<20200220204039.GM4998@pendragon.ideasonboard.com>","date":"2020-02-20T20:40:39","subject":"Re: [libcamera-devel] [PATCH v3 3/6] 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 Thu, Feb 20, 2020 at 04:57:01PM +0000, Kieran Bingham wrote:\n> Provide an optional means to recurse into subdirectories to search for IPA\n> libraries. This allows IPAs contained within their own build directory\n> to be resolved when loading from a non-installed build.\n\ns/resolved/found/ ?\n\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/include/ipa_manager.h | 6 ++-\n> src/libcamera/ipa_manager.cpp | 69 ++++++++++++++++++++++-------\n> 2 files changed, 58 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h\n> index 94d35d9062e4..5dca2c2a7d8d 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 &files,\n> +\t\t unsigned int maxDepth);\n> +\tint addDir(const char *libDir, unsigned int maxDepth = 0);\n> +\tint addPath(const char *path, unsigned int maxDepth = 0);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index d87f2221b00b..c30b4555290f 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -140,16 +140,20 @@ IPAManager *IPAManager::instance()\n> }\n> \n> /**\n> - * \\brief Load IPA modules from a directory\n> - * \\param[in] libDir directory to search for IPA modules\n> + * \\brief Identify shared library objects within a directory\n> + * \\param[in] libDir The directory to search for IPA modules\n\ns/IPA modules/shared objects/ ?\n\n> + * \\param[in] files A vector of paths to shared object library files\n\nthis is an out argument. As such I would move it as the last argument of\nthe function to have input arguments first, followed by output\narguments.\n\n> + * \\param[in] maxDepth The maximum depth of sub-directories to parse\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> + * subdirectories no further than the depth specified by \\a maxDepth.\n\ns/subdirectories/sub-directories/ (for consistency, I don't care much\nwhich one we pick). And this can go on the previous line I think.\n\n> *\n> - * \\return Number of modules loaded by this call, or a negative error code\n> - * otherwise\n> + * Discovered shared objects are added to the files vector.\n\n\"\\a files\" ?\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n\n\"or a negative error code if \\a libdir can't be opened\" ?\n\n> */\n> -int IPAManager::addDir(const char *libDir)\n> +int IPAManager::parseDir(const char *libDir, std::vector &files,\n> +\t\t\t unsigned int maxDepth)\n> {\n> \tstruct dirent *ent;\n> \tDIR *dir;\n> @@ -158,30 +162,64 @@ 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 && maxDepth) {\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/* Recursion is limited to maxDepth. */\n> +\t\t\tparseDir(subdir.c_str(), files, maxDepth - 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> \t\tif (strcmp(&ent->d_name[offset], \".so\"))\n> \t\t\tcontinue;\n> \n> -\t\tpaths.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> +\t\tfiles.push_back(std::string(libDir) + \"/\" + ent->d_name);\n> \t}\n> \tclosedir(dir);\n> \n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Load IPA modules from a directory\n> + * \\param[in] libDir The directory to search for IPA modules\n> + * \\param[in] maxDepth The maximum depth of sub-directories to parse\n\ns/parse/search/ ?\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\nMaybe add in the same paragraph\n\nSub-directories are searched up to a depth of \\a maxDepth. A \\a maxdepth\nvalue of 0 only searches the directory specified in \\a libdir.\n\n> + *\n> + * \\return Number of modules loaded by this call, or a negative error code\n> + * otherwise\n\n\"otherwise\" compared to what ? :-) I think we can drop the error code\nand turn the function return type into unsigned int, as no caller of\naddDir() cares about errors. It will actually simplify the callers that\nwon't have to check for errors anymore, for instance\n\n \t/* Finally try to load IPAs from the installed system path. */\n-\tif (ret > 0)\n-\t\tipaCount += ret;\n+\tipaCount += addDir(IPA_MODULE_DIR);\n\n> + */\n> +int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> +{\n> +\tstd::vector files;\n> +\n> +\tint ret = parseDir(libDir, files, maxDepth);\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> +\tstd::sort(files.begin(), files.end());\n> \n> \tunsigned int count = 0;\n> -\tfor (const std::string &path : paths) {\n> -\t\tIPAModule *ipaModule = new IPAModule(path);\n> +\tfor (const std::string &file : files) {\n> +\t\tIPAModule *ipaModule = new IPAModule(file);\n> \t\tif (!ipaModule->isValid()) {\n> \t\t\tdelete ipaModule;\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tLOG(IPAManager, Debug) << \"Loaded IPA module '\" << path << \"'\";\n> +\t\tLOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> \n> \t\tmodules_.push_back(ipaModule);\n> \t\tcount++;\n> @@ -193,6 +231,7 @@ int IPAManager::addDir(const char *libDir)\n> /**\n> * \\brief Load IPA modules from a search path\n> * \\param[in] path The colon-separated list of directories to load IPA modules from\n> + * \\param[in] maxDepth The maximum number of sub-directories to parse\n\nStill not the maximum number of directories, but the depth :-)\n\n * \\param[in] maxDepth The maximum depth of sub-directories to search\n\nlike above should be fine.\n\nReviewed-by: Laurent Pinchart \n\n> *\n> * This method tries to create an IPAModule instance for every shared object\n> * found in the directories listed in \\a path.\n> @@ -200,7 +239,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 *path)\n> +int IPAManager::addPath(const char *path, unsigned int maxDepth)\n> {\n> \tint ipaCount = 0;\n> \n> @@ -208,7 +247,7 @@ int IPAManager::addPath(const char *path)\n> \t\tif (dir.empty())\n> \t\t\tcontinue;\n> \n> -\t\tint ret = addDir(dir.c_str());\n> +\t\tint ret = addDir(dir.c_str(), maxDepth);\n> \t\tif (ret > 0)\n> \t\t\tipaCount += ret;\n> \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 17CF560434\n\tfor ;\n\tThu, 20 Feb 2020 21:40:59 +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 7F9B1563;\n\tThu, 20 Feb 2020 21:40:58 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582231258;\n\tbh=zf8Cde9TB5M5mZ6aZt2yx2wUwR3vLNTvA+n4tuJqgoE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AnSll9mwukqOJv2WQBI6GshT5A4rC0e9yTATT9uJOfmpbAmMlh1tN5wi7ewXfcF+T\n\t1ol/FjEjr0O9L1J4OVkwM94pe1a3uY1QpNGCkCKSvmY0ryg3/dLsO7HOr+Eqh3iu4r\n\tSRAGSem1hsg5ZX6P6PyFM0YXOb9N+2QHmwC1aen8=","Date":"Thu, 20 Feb 2020 22:40:39 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Message-ID":"<20200220204039.GM4998@pendragon.ideasonboard.com>","References":"<20200220165704.23600-1-kieran.bingham@ideasonboard.com>\n\t<20200220165704.23600-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200220165704.23600-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] 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, 20 Feb 2020 20:40:59 -0000"}}]