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

[Huang Jianan]

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);
>> +                     else
>> +                             ret = 0;
>> +                     free(value);
>> +                     if (ret) {
>> +                             erofs_err("fail to set xattr %s @ nid 
>> %llu",
>> +                                       key, inode->nid | 0ULL);
>> +                             break;
>> +                     }
>> +             }
>> +     }
>> +out:
>> +     free(keylst);
>> +     return ret;
>> +}
>> +
>>   static int erofs_verify_inode_data(struct erofs_inode *inode, int 
>> outfd)
>>   {
>>       struct erofs_map_blocks map = {
>> @@ -900,15 +965,23 @@ static int erofsfsck_check_inode(erofs_nid_t 
>> pnid, erofs_nid_t nid)
>>               goto out;
>>       }
>>
>> -     /* verify xattr field */
>> -     ret = erofs_verify_xattr(&inode);
>> -     if (ret)
>> -             goto out;
>> +     if (!fsckcfg.check_decomp) {
>> +             /* verify xattr field */
>> +             ret = erofs_verify_xattr(&inode);
>> +             if (ret)
>> +                     goto out;
>> +     }
>>
>>       ret = erofsfsck_extract_inode(&inode);
>>       if (ret && ret != -ECANCELED)
>>               goto out;
>>
>> +     if (fsckcfg.check_decomp && fsckcfg.dump_xattrs) {
>> +             ret = erofsfsck_dump_xattrs(&inode);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>>       /* XXXX: the dir depth should be restricted in order to avoid 
>> loops */
>>       if (S_ISDIR(inode.i_mode)) {
>>               struct erofs_dir_context ctx = {
>> @@ -955,6 +1028,7 @@ int main(int argc, char *argv[])
>>       fsckcfg.overwrite = false;
>>       fsckcfg.preserve_owner = fsckcfg.superuser;
>>       fsckcfg.preserve_perms = fsckcfg.superuser;
>> +     fsckcfg.dump_xattrs = true;
>>
>>       err = erofsfsck_parse_options_cfg(argc, argv);
>>       if (err) {
>