On 2023/5/31 11:13, Jingbo Xu wrote:
There's a callback styled xattr parser, i.e. xattr_foreach(), which is
shared among listxattr and getxattr.  Convert it to two separate xattr
parsers for listxattr and getxattr.

Signed-off-by: Jingbo Xu <[email protected]>
---
  fs/erofs/xattr.c | 372 ++++++++++++++++++++---------------------------
  1 file changed, 155 insertions(+), 217 deletions(-)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 074743e2b271..438fcf22b230 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -12,7 +12,7 @@ struct erofs_xattr_iter {
        struct erofs_buf buf;
        void *kaddr;
        erofs_blk_t blkaddr;
-       unsigned int ofs;
+       unsigned int ofs, next_ofs;
char *buffer;
        int buffer_size, buffer_ofs;
@@ -141,183 +141,6 @@ static int erofs_init_inode_xattrs(struct inode *inode)
        return ret;
  }
-/*
- * the general idea for these return values is
- * if    0 is returned, go on processing the current xattr;
- *       1 (> 0) is returned, skip this round to process the next xattr;
- *    -err (< 0) is returned, an error (maybe ENOXATTR) occurred
- *                            and need to be handled
- */
-struct xattr_iter_handlers {
-       int (*entry)(struct erofs_xattr_iter *it, struct erofs_xattr_entry 
*entry);
-       int (*name)(struct erofs_xattr_iter *it, unsigned int processed, char 
*buf,
-                   unsigned int len);
-       int (*alloc_buffer)(struct erofs_xattr_iter *it, unsigned int value_sz);
-       void (*value)(struct erofs_xattr_iter *it, unsigned int processed, char 
*buf,
-                     unsigned int len);
-};
-
-/*
- * Regardless of success or failure, `xattr_foreach' will end up with
- * `ofs' pointing to the next xattr item rather than an arbitrary position.
- */
-static int xattr_foreach(struct erofs_xattr_iter *it,
-                        const struct xattr_iter_handlers *op,
-                        unsigned int *tlimit)
-{
-       struct erofs_xattr_entry entry;
-       unsigned int value_sz, processed, slice;
-       int err;
-
-       /* 0. fixup blkaddr, ofs, ipage */
-       err = erofs_xattr_iter_fixup(it, false);
-       if (err)
-               return err;
-
-       /*
-        * 1. read xattr entry to the memory,
-        *    since we do EROFS_XATTR_ALIGN
-        *    therefore entry should be in the page
-        */
-       entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs);
-       if (tlimit) {
-               unsigned int entry_sz = erofs_xattr_entry_size(&entry);
-
-               /* xattr on-disk corruption: xattr entry beyond xattr_isize */
-               if (*tlimit < entry_sz) {
-                       DBG_BUGON(1);
-                       return -EFSCORRUPTED;
-               }
-               *tlimit -= entry_sz;
-       }
-
-       it->ofs += sizeof(struct erofs_xattr_entry);
-       value_sz = le16_to_cpu(entry.e_value_size);
-
-       /* handle entry */
-       err = op->entry(it, &entry);
-       if (err) {
-               it->ofs += entry.e_name_len + value_sz;
-               goto out;
-       }
-
-       /* 2. handle xattr name (ofs will finally be at the end of name) */
-       processed = 0;
-
-       while (processed < entry.e_name_len) {
-               err = erofs_xattr_iter_fixup(it, true);
-               if (err)
-                       goto out;
-
-               slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
-                             entry.e_name_len - processed);
-
-               /* handle name */
-               err = op->name(it, processed, it->kaddr + it->ofs, slice);
-               if (err) {
-                       it->ofs += entry.e_name_len - processed + value_sz;
-                       goto out;
-               }
-
-               it->ofs += slice;
-               processed += slice;
-       }
-
-       /* 3. handle xattr value */
-       processed = 0;
-
-       if (op->alloc_buffer) {
-               err = op->alloc_buffer(it, value_sz);
-               if (err) {
-                       it->ofs += value_sz;
-                       goto out;
-               }
-       }
-
-       while (processed < value_sz) {
-               err = erofs_xattr_iter_fixup(it, true);
-               if (err)
-                       goto out;
-
-               slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
-                             value_sz - processed);
-               op->value(it, processed, it->kaddr + it->ofs, slice);
-               it->ofs += slice;
-               processed += slice;
-       }
-
-out:
-       /* xattrs should be 4-byte aligned (on-disk constraint) */
-       it->ofs = EROFS_XATTR_ALIGN(it->ofs);
-       return err < 0 ? err : 0;
-}
-
-static int erofs_xattr_long_entrymatch(struct erofs_xattr_iter *it,
-                                      struct erofs_xattr_entry *entry)
-{
-       struct erofs_sb_info *sbi = EROFS_SB(it->sb);
-       struct erofs_xattr_prefix_item *pf = sbi->xattr_prefixes +
-               (entry->e_name_index & EROFS_XATTR_LONG_PREFIX_MASK);
-
-       if (pf >= sbi->xattr_prefixes + sbi->xattr_prefix_count)
-               return -ENOATTR;
-
-       if (it->index != pf->prefix->base_index ||
-           it->name.len != entry->e_name_len + pf->infix_len)
-               return -ENOATTR;
-
-       if (memcmp(it->name.name, pf->prefix->infix, pf->infix_len))
-               return -ENOATTR;
-
-       it->infix_len = pf->infix_len;
-       return 0;
-}
-
-static int xattr_entrymatch(struct erofs_xattr_iter *it,
-                           struct erofs_xattr_entry *entry)
-{
-       /* should also match the infix for long name prefixes */
-       if (entry->e_name_index & EROFS_XATTR_LONG_PREFIX)
-               return erofs_xattr_long_entrymatch(it, entry);
-
-       if (it->index != entry->e_name_index ||
-           it->name.len != entry->e_name_len)
-               return -ENOATTR;
-       it->infix_len = 0;
-       return 0;
-}
-
-static int xattr_namematch(struct erofs_xattr_iter *it,
-                          unsigned int processed, char *buf, unsigned int len)
-{
-       if (memcmp(buf, it->name.name + it->infix_len + processed, len))
-               return -ENOATTR;
-       return 0;
-}
-
-static int xattr_checkbuffer(struct erofs_xattr_iter *it,
-                            unsigned int value_sz)
-{
-       int err = it->buffer_size < value_sz ? -ERANGE : 0;
-
-       it->buffer_ofs = value_sz;
-       return !it->buffer ? 1 : err;
-}
-
-static void xattr_copyvalue(struct erofs_xattr_iter *it,
-                           unsigned int processed,
-                           char *buf, unsigned int len)
-{
-       memcpy(it->buffer + processed, buf, len);
-}
-
-static const struct xattr_iter_handlers find_xattr_handlers = {
-       .entry = xattr_entrymatch,
-       .name = xattr_namematch,
-       .alloc_buffer = xattr_checkbuffer,
-       .value = xattr_copyvalue
-};
-
  static bool erofs_xattr_user_list(struct dentry *dentry)
  {
        return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
@@ -370,20 +193,70 @@ const struct xattr_handler *erofs_xattr_handlers[] = {
        NULL,
  };
-static int xattr_entrylist(struct erofs_xattr_iter *it,
-                          struct erofs_xattr_entry *entry)
+typedef int (*erofs_xattr_body_handler)(struct erofs_xattr_iter *it,
+               unsigned int processed, char *buf, unsigned int len);
+
+static int erofs_xattr_namematch(struct erofs_xattr_iter *it,
+               unsigned int processed, char *buf, unsigned int len)
+{
+       if (memcmp(buf, it->name.name + it->infix_len + processed, len))
+               return -ENOATTR;
+       return 0;
+}
+
+static int erofs_xattr_copy(struct erofs_xattr_iter *it,
+               unsigned int unused, char *buf, unsigned int len)
+{
+       memcpy(it->buffer + it->buffer_ofs, buf, len);
+       it->buffer_ofs += len;
+       return 0;
+}
+
+static int erofs_xattr_body(struct erofs_xattr_iter *it, unsigned int len,
+                           erofs_xattr_body_handler handler)


could we change erofs_xattr_body_handler into a bool (e.g.
bool copy_or_match)?

+{
+       unsigned int slice, processed = 0;
+
+       while (processed < len) {
+               int err = erofs_xattr_iter_fixup(it, true);
+               if (err)
+                       return err;
+
+               slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
+                             len - processed);
+               err = handler(it, processed, it->kaddr + it->ofs, slice);

So that an indirect call could be avoided, as:

                if (copy_or_match) {
                        memcpy(it->buffer + it->buffer_ofs, buf, len);
                        it->buffer_ofs += len;
                } else if (memcmp(buf,
                                it->name.name + it->infix_len + processed, 
len)) {
                        return -ENOATTR;
                }

?

Thanks,
Gao Xiang

Reply via email to