[libcamera-devel] [PATCH v3 1/6] libcamera: utils: Add an internal dirname helper

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Feb 20, 2020 at 04:56:59PM +0000, Kieran Bingham wrote:
> Provide a wrapped dirname call which returns a std::string.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/include/utils.h |  1 +
>  src/libcamera/utils.cpp       | 17 +++++++++++++++++
>  2 files changed, 18 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..3fd3aeaf822a 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -70,6 +70,23 @@ char *secure_getenv(const char *name)
>  #endif
>  }
>  
> +/**
> + * \brief identify the dirname portion of a path

s/identify/Identify/

> + * \param[in] path The full path to parse
> + *

Should we add that this function conforms to the behaviour of the
dirname() function defined by POSIX ?

> + * \returns A string of the directory component of the path

s/returns/return/

> + */
> +std::string dirname(const std::string &path)
> +{
> +	size_t pos = path.rfind('/', path.length());
> +
> +	if (pos != std::string::npos) {
> +		return (path.substr(0, pos));
> +	}

No need for braces and parentheses around path.substr(0, pos).

> +
> +	return path;

To be compatible with the glibc implementation, you should return the
string "." if the path doesn't contain a '/'. Furthemore, if the whole
path string is equal to "/", then you should return "/", while the above
returns an empty string. There's also the case of trailing '/' in the
path that is not handled correctly.

How about the following (only compile-test) ?

/**
 * \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();
	while (pos > 0 && path[pos-1] == '/')
		pos--;

        if (!pos)
		return "/";

	/*
	 * Find the previous slash. If the path contains no non-trailing slash,
	 * return ".".
	 */
	while (pos > 0 && path[pos-1] != '/')
		pos--;

	if (!pos)
		return ".";

	/*
	 * Return the directory name until (but not including) the slash. If
	 * this would result in an empty string, return "/".
	 */
	if (pos == 1)
		return "/";

	return path.substr(0, pos - 1);
}

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

> +}
> +
>  /**
>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
>   *				     InputIt2 first2, InputIt2 last2)

-- 
Regards,

Laurent Pinchart