[PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[Hongzhen Luo]

When the `path` contains symbolic links, erofs_ilookup() does not
function properly. This adds support for symlink files.

Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
---
 lib/namei.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lib/namei.c b/lib/namei.c
index 6f35ee6..dce2991 100644
--- a/lib/namei.c
+++ b/lib/namei.c
@@ -195,6 +195,22 @@ struct nameidata {
 	unsigned int	ftype;
 };
 
+static int link_path_walk(const char *name, struct nameidata *nd);
+
+static int step_into_link(struct nameidata *nd, struct erofs_inode *vi)
+{
+	char buf[EROFS_MAX_BLOCK_SIZE];
+	int err;
+
+	if (vi->i_size > EROFS_MAX_BLOCK_SIZE)
+		return -EINVAL;
+	memset(buf, 0, sizeof(buf));
+	err = erofs_pread(vi, buf, vi->i_size, 0);
+	if (err)
+		return err;
+	return link_path_walk(buf, nd);
+}
+
 int erofs_namei(struct nameidata *nd, const char *name, unsigned int len)
 {
 	erofs_nid_t nid = nd->nid;
@@ -233,6 +249,11 @@ int erofs_namei(struct nameidata *nd, const char *name, unsigned int len)
 			return PTR_ERR(de);
 
 		if (de) {
+			vi.nid = de->nid;
+			ret = erofs_read_inode_from_disk(&vi);
+			if (S_ISLNK(vi.i_mode)) {
+				return step_into_link(nd, &vi);
+			}
 			nd->nid = le64_to_cpu(de->nid);
 			return 0;
 		}
@@ -243,7 +264,8 @@ int erofs_namei(struct nameidata *nd, const char *name, unsigned int len)
 
 static int link_path_walk(const char *name, struct nameidata *nd)
 {
-	nd->nid = nd->sbi->root_nid;
+	if (*name == '/')
+		nd->nid = nd->sbi->root_nid;
 
 	while (*name == '/')
 		name++;
@@ -274,6 +296,7 @@ int erofs_ilookup(const char *path, struct erofs_inode *vi)
 	int ret;
 	struct nameidata nd = { .sbi = vi->sbi };
 
+	nd.nid = nd.sbi->root_nid;
 	ret = link_path_walk(path, &nd);
 	if (ret)
 		return ret;
-- 
2.43.5

Re: [PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[Gao Xiang]

On 2024/8/11 16:09, Hongzhen Luo wrote:
> When the `path` contains symbolic links, erofs_ilookup() does not
> function properly. This adds support for symlink files.

Can you explain what's the use cases of this patch?

It seems both erofsfuse and fsck.erofs --extract don't need this.

> 
> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
> ---
>   lib/namei.c | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/namei.c b/lib/namei.c
> index 6f35ee6..dce2991 100644
> --- a/lib/namei.c
> +++ b/lib/namei.c
> @@ -195,6 +195,22 @@ struct nameidata {
>   	unsigned int	ftype;
>   };
>   
> +static int link_path_walk(const char *name, struct nameidata *nd);
> +
> +static int step_into_link(struct nameidata *nd, struct erofs_inode *vi)
> +{
> +	char buf[EROFS_MAX_BLOCK_SIZE];
> +	int err;
> +
> +	if (vi->i_size > EROFS_MAX_BLOCK_SIZE)
> +		return -EINVAL;

No, symlink size is independent to EROFS_MAX_BLOCK_SIZE, currently
it's hard-code as 4096.

> +	memset(buf, 0, sizeof(buf));
> +	err = erofs_pread(vi, buf, vi->i_size, 0);
> +	if (err)
> +		return err;
> +	return link_path_walk(buf, nd);
> +}
> +
>   int erofs_namei(struct nameidata *nd, const char *name, unsigned int len)
>   {
>   	erofs_nid_t nid = nd->nid;
> @@ -233,6 +249,11 @@ int erofs_namei(struct nameidata *nd, const char *name, unsigned int len)
>   			return PTR_ERR(de);
>   
>   		if (de) {
> +			vi.nid = de->nid;
> +			ret = erofs_read_inode_from_disk(&vi);
> +			if (S_ISLNK(vi.i_mode)) {
> +				return step_into_link(nd, &vi);
> +			}

Why need brace here?

Thanks,
Gao Xiang

Re: [PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[Hongzhen Luo]

On 2024/8/12 10:24, Gao Xiang wrote:
>
>
> On 2024/8/11 16:09, Hongzhen Luo wrote:
>> When the `path` contains symbolic links, erofs_ilookup() does not
>> function properly. This adds support for symlink files.
>
> Can you explain what's the use cases of this patch?
>
> It seems both erofsfuse and fsck.erofs --extract don't need this.
>
Some third-party applications (such as Alibaba DADI) require obtaining 
block mapping information of files based on their paths using liberofs. 
When file paths include symbolic links, the current erofs_ilookup() 
function fails to correctly locate the inode. This submission enhances 
erofs_ilookup()'s support for symbolic link files.
>>
>> Signed-off-by: Hongzhen Luo <hongzhen@linux.alibaba.com>
>> ---
>>   lib/namei.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/namei.c b/lib/namei.c
>> index 6f35ee6..dce2991 100644
>> --- a/lib/namei.c
>> +++ b/lib/namei.c
>> @@ -195,6 +195,22 @@ struct nameidata {
>>       unsigned int    ftype;
>>   };
>>   +static int link_path_walk(const char *name, struct nameidata *nd);
>> +
>> +static int step_into_link(struct nameidata *nd, struct erofs_inode *vi)
>> +{
>> +    char buf[EROFS_MAX_BLOCK_SIZE];
>> +    int err;
>> +
>> +    if (vi->i_size > EROFS_MAX_BLOCK_SIZE)
>> +        return -EINVAL;
>
> No, symlink size is independent to EROFS_MAX_BLOCK_SIZE, currently
> it's hard-code as 4096.
>
Okay, I will make the corresponding modifications in the next version.
>> +    memset(buf, 0, sizeof(buf));
>> +    err = erofs_pread(vi, buf, vi->i_size, 0);
>> +    if (err)
>> +        return err;
>> +    return link_path_walk(buf, nd);
>> +}
>> +
>>   int erofs_namei(struct nameidata *nd, const char *name, unsigned 
>> int len)
>>   {
>>       erofs_nid_t nid = nd->nid;
>> @@ -233,6 +249,11 @@ int erofs_namei(struct nameidata *nd, const char 
>> *name, unsigned int len)
>>               return PTR_ERR(de);
>>             if (de) {
>> +            vi.nid = de->nid;
>> +            ret = erofs_read_inode_from_disk(&vi);
>> +            if (S_ISLNK(vi.i_mode)) {
>> +                return step_into_link(nd, &vi);
>> +            }
>
> Why need brace here?

Yes, the braces are not necessary, and I will clean them up later.

>
> Thanks,
> Gao Xiang

---

Thanks,

Hongzhen Luo

Re: [PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[Gao Xiang]

On 2024/8/12 10:50, Hongzhen Luo wrote:
> 
> On 2024/8/12 10:24, Gao Xiang wrote:
>>
>>
>> On 2024/8/11 16:09, Hongzhen Luo wrote:
>>> When the `path` contains symbolic links, erofs_ilookup() does not
>>> function properly. This adds support for symlink files.
>>
>> Can you explain what's the use cases of this patch?
>>
>> It seems both erofsfuse and fsck.erofs --extract don't need this.
>>
> Some third-party applications (such as Alibaba DADI) require obtaining block mapping information of files based on their paths using liberofs. When file paths include symbolic links, the current erofs_ilookup() function fails to correctly locate the inode. This submission enhances erofs_ilookup()'s support for symbolic link files.

Why it cannot be implemented in the application itself?
Following block mapping and obtain the symlinked file block
mapping is weird for erofs itself to resolve.

Thanks,
Gao Xiang

Re: [PATCH] erofs-utils: lib: add support for symlink file in erofs_ilookup()

[Hongzhen Luo]

On 2024/8/12 11:03, Gao Xiang wrote:
>
>
> On 2024/8/12 10:50, Hongzhen Luo wrote:
>>
>> On 2024/8/12 10:24, Gao Xiang wrote:
>>>
>>>
>>> On 2024/8/11 16:09, Hongzhen Luo wrote:
>>>> When the `path` contains symbolic links, erofs_ilookup() does not
>>>> function properly. This adds support for symlink files.
>>>
>>> Can you explain what's the use cases of this patch?
>>>
>>> It seems both erofsfuse and fsck.erofs --extract don't need this.
>>>
>> Some third-party applications (such as Alibaba DADI) require 
>> obtaining block mapping information of files based on their paths 
>> using liberofs. When file paths include symbolic links, the current 
>> erofs_ilookup() function fails to correctly locate the inode. This 
>> submission enhances erofs_ilookup()'s support for symbolic link files.
>
> Why it cannot be implemented in the application itself?
> Following block mapping and obtain the symlinked file block
> mapping is weird for erofs itself to resolve.
>
> Thanks,
> Gao Xiang

That makes sense. Thanks for pointing it out, and please disregard this 
patch.

---

Thanks,

Hongzhen Luo