[libcamera-devel] [PATCH v4 1/6] libcamera: utils: Add a C++ dirname implementation

[Kieran Bingham]

Hi Laurent,

On 22/02/2020 10:27, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Feb 21, 2020 at 04:31:25PM +0000, Kieran Bingham wrote:
>> Provide a std::string based implementation which conforms to the
>> behaviour of the dirname() fucntion defined by POSIX.
>>
>> Tests are added to cover expected corner cases of the implementation.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/include/utils.h |  1 +
>>  src/libcamera/utils.cpp       | 48 +++++++++++++++++++++++++++++++
>>  test/utils.cpp                | 54 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 103 insertions(+)
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index 080ea6614de0..940597760ee2 100644
>> --- a/src/libcamera/include/utils.h
>> +++ b/src/libcamera/include/utils.h
>> @@ -33,6 +33,7 @@ namespace utils {
>>  const char *basename(const char *path);
>>  
>>  char *secure_getenv(const char *name);
>> +std::string dirname(const std::string &path);
>>  
>>  template<class InputIt1, class InputIt2>
>>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> index 453e3b3b5995..f566e88cec5b 100644
>> --- a/src/libcamera/utils.cpp
>> +++ b/src/libcamera/utils.cpp
>> @@ -70,6 +70,54 @@ char *secure_getenv(const char *name)
>>  #endif
>>  }
>>  
>> +/**
>> + * \brief Identify the dirname portion of a path
>> + * \param[in] path The full path to parse
>> + *
>> + * This function conforms with the behaviour of the %dirname() function as
>> + * defined by POSIX.
>> + *
>> + * \return A string of the directory component of the path
>> + */
>> +std::string dirname(const std::string &path)
>> +{
>> +	if (path.empty())
>> +		return ".";
>> +
>> +	/*
>> +	 * Skip all trailing slashes. If the path is only made of slashes,
>> +	 * return "/".
>> +	 */
>> +	size_t pos = path.size() - 1;
>> +	while (path[pos] == '/') {
>> +		if (!pos)
>> +			return "/";
>> +		pos--;
>> +	}
>> +
>> +	/*
>> +	 * Find the previous slash. If the path contains no non-trailing slash,
>> +	 * return ".".
>> +	 */
>> +	while (path[pos] != '/') {
>> +		if (!pos)
>> +			return ".";
>> +		pos--;
>> +	}
>> +
>> +	/*
>> +	 * Return the directory name up to (but not including) any trailing
>> +	 * slash. If this would result in an empty string, return "/".
>> +	 */
>> +	while (path[pos] == '/') {
>> +		if (!pos)
>> +			return "/";
>> +		pos--;
>> +	}
>> +
>> +	return path.substr(0, pos + 1);
>> +}
>> +
>>  /**
>>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
>>   *				     InputIt2 first2, InputIt2 last2)
>> diff --git a/test/utils.cpp b/test/utils.cpp
>> index db1fbdde847d..e4184e39ce32 100644
>> --- a/test/utils.cpp
>> +++ b/test/utils.cpp
>> @@ -19,6 +19,56 @@ using namespace libcamera;
>>  class UtilsTest : public Test
>>  {
>>  protected:
>> +	int testDirname()
>> +	{
>> +		std::vector<std::string> paths = {
> 
> You can make this const.

Ah yes

> 
>> +			"",
>> +			"///",
>> +			"/bin",
>> +			"/usr/bin",
>> +			"//etc////",
>> +			"//tmp//d//",
>> +			"current_file",
>> +			"./current_file",
>> +			"./current_dir/",
>> +			"current_dir/",
>> +		};
>> +
>> +		std::vector<std::string> expected = {
> 
> And this too.
> 
>> +			".",
>> +			"/",
>> +			"/",
>> +			"/usr",
>> +			"/",
>> +			"//tmp",
>> +			".",
>> +			".",
>> +			".",
>> +			".",
>> +		};
> 
> I was going to propose using the libc-provided dirname() function to
> check for correctness instead of hardcoding the results, but that would
> create a dependency on the libc implementation conforming with POSIX,
> which may not be guaranteed on non-glibc platforms. Let's thus keep it
> as-is.

Indeed, I wanted direct expected results hardcoded for validation.

I also liked your utils::split test implementation, and I was tempted to
make this a vector of struct { char * path, char * expected } , but I
just went with this for speed, and because it's nice to show the whole
results in the event of a failure.


>> +
>> +		std::vector<std::string> results;
>> +
>> +		for (const auto &path : paths)
>> +			results.push_back(utils::dirname(path));
>> +
>> +		if (results != expected) {
>> +			cerr << "utils::dirname() tests failed" << endl;
>> +
>> +			cerr << "expected: " << endl;
>> +			for (const auto &path : expected)
>> +				cerr << " " << path << endl;
> 
> "\t" instead of " " for additional clarity ?

Sure, Mostly this code came in useful while I was fixing the
implementation so I /hope/ it will never be broken, and thus this code
shouldn't ever print (unless someone extends the tss?)

> 
>> +
>> +			cerr << "results: " << endl;
>> +			for (const auto &path : results)
>> +				cerr << " " << path << endl;
> 
> Here too ?
> 
>> +
>> +			return TestFail;
>> +		}
>> +
>> +		return 0;
> 
> 		return TestPass;
> 
>> +	}
>> +
>>  	int run()
>>  	{
>>  		/* utils::hex() test. */
>> @@ -71,6 +121,10 @@ protected:
>>  			return TestFail;
>>  		}
>>  
>> +		/* utils::dirname() tests. */
>> +		if (testDirname())
> 
> 		if (testDirname() != TestPass)
> 
> ?


Yes, that would be better in case the TestPass/TestFail values ever gets
changed underneath...

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Thanks, fixups applied.


> 
>> +			return TestFail;
>> +
>>  		return TestPass;
>>  	}
>>  };
> 

-- 
Regards
--
Kieran