[External Mail]Re: [PATCH v5] erofs-utils: fsck: introduce exporting xattrs

[Hongzhen Luo]

On 2024/9/6 11:09, Gao Xiang wrote:
> On 2024/9/6 10:46, Huang Jianan via Linux-erofs wrote:
>> On 2024/9/6 10:31, Gao Xiang wrote:
>>>
>>> Hi Jianan,
>>>
>>> On 2024/9/6 10:22, Hongzhen Luo wrote:
>>>> The current `fsck --extract` does not support exporting the extended
>>>> attributes of files. This patch adds `--xattrs` option to dump the
>>>> extended attributes, as well as the `--no-xattrs` option to not dump
>>>> the extended attributes.
>>>>
>>>> Signed-off-by: Hongzhen Luo <hongzhen at linux.alibaba.com>
>>>
>>> Could you confirm this version?
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>
>> LGTM.
>>
>> Reviewed-by: Jianan Huang <huangjianan at xiaomi.com>
>> Thanks,
>>
>>>> ---
>>>> v5: Adjust the help information.
>>>> v4: https://lore.kernel.org/all/20240905113723.1937634-1-
>>>> hongzhen at linux.alibaba.com/
>>>> v3: https://lore.kernel.org/all/20240903113729.3539578-1-
>>>> hongzhen at linux.alibaba.com/
>>>> v2: https://lore.kernel.org/all/20240903085643.3393012-1-
>>>> hongzhen at linux.alibaba.com/
>>>> v1: https://lore.kernel.org/all/20240903073517.3311407-1-
>>>> hongzhen at linux.alibaba.com/
>>>> ---
>>>>    fsck/main.c | 82 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>    1 file changed, 78 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fsck/main.c b/fsck/main.c
>>>> index 28f1e7e..d110cdb 100644
>>>> --- a/fsck/main.c
>>>> +++ b/fsck/main.c
>>>> @@ -9,6 +9,7 @@
>>>>    #include <utime.h>
>>>>    #include <unistd.h>
>>>>    #include <sys/stat.h>
>>>> +#include <sys/xattr.h>
>>>>    #include "erofs/print.h"
>>>>    #include "erofs/compress.h"
>>>>    #include "erofs/decompress.h"
>>>> @@ -31,6 +32,7 @@ struct erofsfsck_cfg {
>>>>        bool overwrite;
>>>>        bool preserve_owner;
>>>>        bool preserve_perms;
>>>> +     bool dump_xattrs;
>>>>    };
>>>>    static struct erofsfsck_cfg fsckcfg;
>>>>
>>>> @@ -48,6 +50,8 @@ static struct option long_options[] = {
>>>>        {"no-preserve-owner", no_argument, 0, 10},
>>>>        {"no-preserve-perms", no_argument, 0, 11},
>>>>        {"offset", required_argument, 0, 12},
>>>> +     {"xattrs", no_argument, 0, 13},
>>>> +     {"no-xattrs", no_argument, 0, 14},
>>>>        {0, 0, 0, 0},
>>>>    };
>>>>
>>>> @@ -98,6 +102,7 @@ static void usage(int argc, char **argv)
>>>>                " --extract[=X]          check if all files are well
>>>> encoded, optionally\n"
>>>>                "                        extract to X\n"
>>>>                " --offset=#             skip # bytes at the beginning
>>>> of IMAGE\n"
>>>> +             " --[no-]xattrs          whether to dump extended
>>>> attributes (default on)\n"
>>>>                "\n"
>>>>                " -a, -A, -y             no-op, for compatibility with
>>>> fsck of other filesystems\n"
>>>>                "\n"
>>>> @@ -225,6 +230,12 @@ static int erofsfsck_parse_options_cfg(int argc,
>>>> char **argv)
>>>>                                return -EINVAL;
>>>>                        }
>>>>                        break;
>>>> +             case 13:
>>>> +                     fsckcfg.dump_xattrs = true;
>>>> +                     break;
>>>> +             case 14:
>>>> +                     fsckcfg.dump_xattrs = false;
>>>> +                     break;
>>>>                default:
>>>>                        return -EINVAL;
>>>>                }
>>>> @@ -411,6 +422,60 @@ out:
>>>>        return ret;
>>>>    }
>>>>
>>>> +static int erofsfsck_dump_xattrs(struct erofs_inode *inode)
>>>> +{
>>>> +     char *keylst, *key;
>>>> +     ssize_t kllen;
>>>> +     int ret;
>>>> +
>>>> +     kllen = erofs_listxattr(inode, NULL, 0);
>>>> +     if (kllen <= 0)
>>>> +             return kllen;
>>>> +     keylst = malloc(kllen);
>>>> +     if (!keylst)
>>>> +             return -ENOMEM;
>>>> +     ret = erofs_listxattr(inode, keylst, kllen);
>>>> +     if (ret < 0)
>>>> +             goto out;
>>>> +     for (key = keylst; key < keylst + kllen; key += strlen(key) + 
>>>> 1) {
>>>> +             void *value = NULL;
>>>> +             size_t size = 0;
>>>> +
>>>> +             ret = erofs_getxattr(inode, key, NULL, 0);
>>>> +             if (ret < 0)
>>>> +                     break;
>>>> +             if (ret) {
>>>> +                     size = ret;
>>>> +                     value = malloc(size);
>>>> +                     if (!value) {
>>>> +                             ret = -ENOMEM;
>>>> +                             break;
>>>> +                     }
>>>> +                     ret = erofs_getxattr(inode, key, value, size);
>>>> +                     if (ret < 0) {
>>>> +                             erofs_err("fail to get xattr %s @ nid
>>>> %llu",
>>>> +                                       key, inode->nid | 0ULL);
>>>> +                             free(value);
>>>> +                             break;
>>>> +                     }
>>>> +                     if (fsckcfg.extract_path)
>>>> +                             ret = setxattr(fsckcfg.extract_path,
>>>> key, value,
>>>> +                                            size, 0);
>
> I still have some comments,
>
>  - need to update fsck manpages together;
>
>  - If `erofsfsck.superuser` is false, I guess we need to ignore
>    non-`user.` namespace xattrs.
>    If `erofsfsck.superuser` is true, we could dump all xattrs then.
>
>  - you should use lsetxattr instead of setxattr.
>
> Thanks,
> Gao Xiang
Sure, I will do these in the next patch.

Thanks,

Hongzhen