[PATCH] erofs-utils: add dump.erofs to .gitignore

[PATCH] erofs-utils: add dump.erofs to .gitignore

[Huang Jianan via Linux-erofs]

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 7bc3f58..27403d4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,3 +29,4 @@ stamp-h
 stamp-h1
 /mkfs/mkfs.erofs
 /fuse/erofsfuse
+/dump/dump.erofs
-- 
2.25.1

[PATCH] erofs-utils: sort shared xattr

[Huang Jianan via Linux-erofs]

Sort shared xattr before writing to disk to ensure the consistency
of reproducible builds.
---
 lib/xattr.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/lib/xattr.c b/lib/xattr.c
index 196133a..f17e57e 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -171,7 +171,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
 	/* allocate key-value buffer */
 	len[0] = keylen - prefixlen;
 
-	kvbuf = malloc(len[0] + len[1]);
+	kvbuf = malloc(len[0] + len[1] + 1);
 	if (!kvbuf)
 		return ERR_PTR(-ENOMEM);
 	memcpy(kvbuf, key + prefixlen, len[0]);
@@ -196,6 +196,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
 			len[1] = ret;
 		}
 	}
+	kvbuf[len[0] + len[1]] = '\0';
 	return get_xattritem(prefix, kvbuf, len);
 }
 
@@ -398,7 +399,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
 	len[0] = sizeof("capability") - 1;
 	len[1] = sizeof(caps);
 
-	kvbuf = malloc(len[0] + len[1]);
+	kvbuf = malloc(len[0] + len[1] + 1);
 	if (!kvbuf)
 		return -ENOMEM;
 
@@ -409,6 +410,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
 	caps.data[1].permitted = (u32) (capabilities >> 32);
 	caps.data[1].inheritable = 0;
 	memcpy(kvbuf + len[0], &caps, len[1]);
+	kvbuf[len[0] + len[1]] = '\0';
 
 	item = get_xattritem(EROFS_XATTR_INDEX_SECURITY, kvbuf, len);
 	if (IS_ERR(item))
@@ -562,13 +564,23 @@ static struct erofs_bhops erofs_write_shared_xattrs_bhops = {
 	.flush = erofs_bh_flush_write_shared_xattrs,
 };
 
+static int comp_xattr_item(const void *a, const void *b)
+{
+	const struct xattr_item *ia, *ib;
+
+	ia = (*((const struct inode_xattr_node **)a))->item;
+	ib = (*((const struct inode_xattr_node **)b))->item;
+
+	return strcmp(ia->kvbuf, ib->kvbuf);
+}
+
 int erofs_build_shared_xattrs_from_path(const char *path)
 {
 	int ret;
 	struct erofs_buffer_head *bh;
-	struct inode_xattr_node *node, *n;
+	struct inode_xattr_node *node, *n, **sorted_n;
 	char *buf;
-	unsigned int p;
+	unsigned int p, i;
 	erofs_off_t off;
 
 	/* check if xattr or shared xattr is disabled */
@@ -606,6 +618,20 @@ int erofs_build_shared_xattrs_from_path(const char *path)
 	off %= EROFS_BLKSIZ;
 	p = 0;
 
+	sorted_n = malloc(shared_xattrs_count * sizeof(n));
+	if (!sorted_n)
+		return -ENOMEM;
+	i = 0;
+	list_for_each_entry_safe(node, n, &shared_xattrs_list, list) {
+		list_del(&node->list);
+		sorted_n[i++] = node;
+	}
+	DBG_BUGON(i != shared_xattrs_count);
+	qsort(sorted_n, shared_xattrs_count, sizeof(n), comp_xattr_item);
+	for (i = 0; i < shared_xattrs_count; i++)
+		list_add_tail(&sorted_n[i]->list, &shared_xattrs_list);
+	free(sorted_n);
+
 	list_for_each_entry_safe(node, n, &shared_xattrs_list, list) {
 		struct xattr_item *const item = node->item;
 		const struct erofs_xattr_entry entry = {
-- 
2.25.1

Re: [PATCH] erofs-utils: add dump.erofs to .gitignore

[Gao Xiang]

On Thu, Oct 21, 2021 at 10:58:46AM +0800, Huang Jianan via Linux-erofs wrote:

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>  .gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitignore b/.gitignore
> index 7bc3f58..27403d4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -29,3 +29,4 @@ stamp-h
>  stamp-h1
>  /mkfs/mkfs.erofs
>  /fuse/erofsfuse
> +/dump/dump.erofs
> -- 
> 2.25.1

Re: [PATCH] erofs-utils: sort shared xattr

[Gao Xiang]

Hi Jianan,

On Thu, Oct 21, 2021 at 10:58:47AM +0800, Huang Jianan via Linux-erofs wrote:
> Sort shared xattr before writing to disk to ensure the consistency
> of reproducible builds.

How about adding it as an option?

> ---
>  lib/xattr.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/xattr.c b/lib/xattr.c
> index 196133a..f17e57e 100644
> --- a/lib/xattr.c
> +++ b/lib/xattr.c
> @@ -171,7 +171,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
>  	/* allocate key-value buffer */
>  	len[0] = keylen - prefixlen;
>  
> -	kvbuf = malloc(len[0] + len[1]);
> +	kvbuf = malloc(len[0] + len[1] + 1);
>  	if (!kvbuf)
>  		return ERR_PTR(-ENOMEM);
>  	memcpy(kvbuf, key + prefixlen, len[0]);
> @@ -196,6 +196,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
>  			len[1] = ret;
>  		}
>  	}
> +	kvbuf[len[0] + len[1]] = '\0';
>  	return get_xattritem(prefix, kvbuf, len);
>  }
>  
> @@ -398,7 +399,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
>  	len[0] = sizeof("capability") - 1;
>  	len[1] = sizeof(caps);
>  
> -	kvbuf = malloc(len[0] + len[1]);
> +	kvbuf = malloc(len[0] + len[1] + 1);
>  	if (!kvbuf)
>  		return -ENOMEM;
>  
> @@ -409,6 +410,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
>  	caps.data[1].permitted = (u32) (capabilities >> 32);
>  	caps.data[1].inheritable = 0;
>  	memcpy(kvbuf + len[0], &caps, len[1]);
> +	kvbuf[len[0] + len[1]] = '\0';
>  
>  	item = get_xattritem(EROFS_XATTR_INDEX_SECURITY, kvbuf, len);
>  	if (IS_ERR(item))
> @@ -562,13 +564,23 @@ static struct erofs_bhops erofs_write_shared_xattrs_bhops = {
>  	.flush = erofs_bh_flush_write_shared_xattrs,
>  };
>  
> +static int comp_xattr_item(const void *a, const void *b)
> +{
> +	const struct xattr_item *ia, *ib;
> +
> +	ia = (*((const struct inode_xattr_node **)a))->item;
> +	ib = (*((const struct inode_xattr_node **)b))->item;
> +
> +	return strcmp(ia->kvbuf, ib->kvbuf);

could we use strncmp (len[0] + len[1]) instead?

Thanks,
Gao Xiang

Re: [PATCH] erofs-utils: add dump.erofs to .gitignore

[Gao Xiang]

On Thu, Oct 21, 2021 at 03:23:46PM +0800, Gao Xiang wrote:
> On Thu, Oct 21, 2021 at 10:58:46AM +0800, Huang Jianan via Linux-erofs wrote:
> 
> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

In addition, lack of "Signed-off-by:", please help add in the next
version.

(It'd be better to add some commit message in general, even the
 modification is quite small....)

> 
> Thanks,
> Gao Xiang
> 
> > ---
> >  .gitignore | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 7bc3f58..27403d4 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -29,3 +29,4 @@ stamp-h
> >  stamp-h1
> >  /mkfs/mkfs.erofs
> >  /fuse/erofsfuse
> > +/dump/dump.erofs
> > -- 
> > 2.25.1

[PATCH v2] erofs-utils: add dump.erofs to .gitignore

[Huang Jianan via Linux-erofs]

The compiled product should be added to gitignore.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 7bc3f58..27403d4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,3 +29,4 @@ stamp-h
 stamp-h1
 /mkfs/mkfs.erofs
 /fuse/erofsfuse
+/dump/dump.erofs
-- 
2.25.1

Re: [PATCH] erofs-utils: sort shared xattr

[Huang Jianan via Linux-erofs]

在 2021/10/21 15:25, Gao Xiang 写道:
> Hi Jianan,
>
> On Thu, Oct 21, 2021 at 10:58:47AM +0800, Huang Jianan via Linux-erofs wrote:
>> Sort shared xattr before writing to disk to ensure the consistency
>> of reproducible builds.
> How about adding it as an option?

Can we consider turning on this by default abd add some test cases to 
ensure that xattr
functionality?  It seems that this part of the modification has no 
effect on the overall
function.

>> ---
>>   lib/xattr.c | 34 ++++++++++++++++++++++++++++++----
>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/xattr.c b/lib/xattr.c
>> index 196133a..f17e57e 100644
>> --- a/lib/xattr.c
>> +++ b/lib/xattr.c
>> @@ -171,7 +171,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
>>   	/* allocate key-value buffer */
>>   	len[0] = keylen - prefixlen;
>>   
>> -	kvbuf = malloc(len[0] + len[1]);
>> +	kvbuf = malloc(len[0] + len[1] + 1);
>>   	if (!kvbuf)
>>   		return ERR_PTR(-ENOMEM);
>>   	memcpy(kvbuf, key + prefixlen, len[0]);
>> @@ -196,6 +196,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
>>   			len[1] = ret;
>>   		}
>>   	}
>> +	kvbuf[len[0] + len[1]] = '\0';
>>   	return get_xattritem(prefix, kvbuf, len);
>>   }
>>   
>> @@ -398,7 +399,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
>>   	len[0] = sizeof("capability") - 1;
>>   	len[1] = sizeof(caps);
>>   
>> -	kvbuf = malloc(len[0] + len[1]);
>> +	kvbuf = malloc(len[0] + len[1] + 1);
>>   	if (!kvbuf)
>>   		return -ENOMEM;
>>   
>> @@ -409,6 +410,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
>>   	caps.data[1].permitted = (u32) (capabilities >> 32);
>>   	caps.data[1].inheritable = 0;
>>   	memcpy(kvbuf + len[0], &caps, len[1]);
>> +	kvbuf[len[0] + len[1]] = '\0';
>>   
>>   	item = get_xattritem(EROFS_XATTR_INDEX_SECURITY, kvbuf, len);
>>   	if (IS_ERR(item))
>> @@ -562,13 +564,23 @@ static struct erofs_bhops erofs_write_shared_xattrs_bhops = {
>>   	.flush = erofs_bh_flush_write_shared_xattrs,
>>   };
>>   
>> +static int comp_xattr_item(const void *a, const void *b)
>> +{
>> +	const struct xattr_item *ia, *ib;
>> +
>> +	ia = (*((const struct inode_xattr_node **)a))->item;
>> +	ib = (*((const struct inode_xattr_node **)b))->item;
>> +
>> +	return strcmp(ia->kvbuf, ib->kvbuf);
> could we use strncmp (len[0] + len[1]) instead?

It seems that strncmp can't guarantee the order since ia and ib has 
different len. We
have ensure kvbuf[len[0] + len[1]] = '\0', Is there anything else to 
consider?

Thanks,
Jianan

> Thanks,
> Gao Xiang

Re: [PATCH] erofs-utils: sort shared xattr

[Gao Xiang]

On Thu, Oct 21, 2021 at 05:28:25PM +0800, Huang Jianan wrote:
> 
> 
> 在 2021/10/21 15:25, Gao Xiang 写道:
> > Hi Jianan,
> > 
> > On Thu, Oct 21, 2021 at 10:58:47AM +0800, Huang Jianan via Linux-erofs wrote:
> > > Sort shared xattr before writing to disk to ensure the consistency
> > > of reproducible builds.
> > How about adding it as an option?
> 
> Can we consider turning on this by default abd add some test cases to ensure
> that xattr
> functionality?  It seems that this part of the modification has no effect on
> the overall
> function.

Ok, that is fine with me as well.

> 
> > > ---
> > >   lib/xattr.c | 34 ++++++++++++++++++++++++++++++----
> > >   1 file changed, 30 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/lib/xattr.c b/lib/xattr.c
> > > index 196133a..f17e57e 100644
> > > --- a/lib/xattr.c
> > > +++ b/lib/xattr.c
> > > @@ -171,7 +171,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
> > >   	/* allocate key-value buffer */
> > >   	len[0] = keylen - prefixlen;
> > > -	kvbuf = malloc(len[0] + len[1]);
> > > +	kvbuf = malloc(len[0] + len[1] + 1);
> > >   	if (!kvbuf)
> > >   		return ERR_PTR(-ENOMEM);
> > >   	memcpy(kvbuf, key + prefixlen, len[0]);
> > > @@ -196,6 +196,7 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
> > >   			len[1] = ret;
> > >   		}
> > >   	}
> > > +	kvbuf[len[0] + len[1]] = '\0';
> > >   	return get_xattritem(prefix, kvbuf, len);
> > >   }
> > > @@ -398,7 +399,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
> > >   	len[0] = sizeof("capability") - 1;
> > >   	len[1] = sizeof(caps);
> > > -	kvbuf = malloc(len[0] + len[1]);
> > > +	kvbuf = malloc(len[0] + len[1] + 1);
> > >   	if (!kvbuf)
> > >   		return -ENOMEM;
> > > @@ -409,6 +410,7 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
> > >   	caps.data[1].permitted = (u32) (capabilities >> 32);
> > >   	caps.data[1].inheritable = 0;
> > >   	memcpy(kvbuf + len[0], &caps, len[1]);
> > > +	kvbuf[len[0] + len[1]] = '\0';
> > >   	item = get_xattritem(EROFS_XATTR_INDEX_SECURITY, kvbuf, len);
> > >   	if (IS_ERR(item))
> > > @@ -562,13 +564,23 @@ static struct erofs_bhops erofs_write_shared_xattrs_bhops = {
> > >   	.flush = erofs_bh_flush_write_shared_xattrs,
> > >   };
> > > +static int comp_xattr_item(const void *a, const void *b)
> > > +{
> > > +	const struct xattr_item *ia, *ib;
> > > +
> > > +	ia = (*((const struct inode_xattr_node **)a))->item;
> > > +	ib = (*((const struct inode_xattr_node **)b))->item;
> > > +
> > > +	return strcmp(ia->kvbuf, ib->kvbuf);
> > could we use strncmp (len[0] + len[1]) instead?
> 
> It seems that strncmp can't guarantee the order since ia and ib has
> different len. We
> have ensure kvbuf[len[0] + len[1]] = '\0', Is there anything else to
> consider?

you could min(xa, xb) and strncmp first. If strncmp returns 0, compare
length then.

Thanks,
Gao Xiang

> 
> Thanks,
> Jianan
> 
> > Thanks,
> > Gao Xiang

[PATCH v2 1/2] erofs-utils: sort shared xattr

[Huang Jianan via Linux-erofs]

Sort shared xattr before writing to disk to ensure the consistency
of reproducible builds.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
---
since v1:
- use strncmp instead of strcmp.

 lib/xattr.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/lib/xattr.c b/lib/xattr.c
index 196133a..fd998cd 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -562,13 +562,31 @@ static struct erofs_bhops erofs_write_shared_xattrs_bhops = {
 	.flush = erofs_bh_flush_write_shared_xattrs,
 };
 
+static int comp_xattr_item(const void *a, const void *b)
+{
+	const struct xattr_item *ia, *ib;
+	unsigned int la, lb;
+	int ret;
+
+	ia = (*((const struct inode_xattr_node **)a))->item;
+	ib = (*((const struct inode_xattr_node **)b))->item;
+	la = ia->len[0] + ia->len[1];
+	lb = ib->len[0] + ib->len[1];
+
+	ret = strncmp(ia->kvbuf, ib->kvbuf, min(la, lb));
+	if (ret != 0)
+		return ret;
+
+	return la > lb;
+}
+
 int erofs_build_shared_xattrs_from_path(const char *path)
 {
 	int ret;
 	struct erofs_buffer_head *bh;
-	struct inode_xattr_node *node, *n;
+	struct inode_xattr_node *node, *n, **sorted_n;
 	char *buf;
-	unsigned int p;
+	unsigned int p, i;
 	erofs_off_t off;
 
 	/* check if xattr or shared xattr is disabled */
@@ -606,6 +624,20 @@ int erofs_build_shared_xattrs_from_path(const char *path)
 	off %= EROFS_BLKSIZ;
 	p = 0;
 
+	sorted_n = malloc(shared_xattrs_count * sizeof(n));
+	if (!sorted_n)
+		return -ENOMEM;
+	i = 0;
+	list_for_each_entry_safe(node, n, &shared_xattrs_list, list) {
+		list_del(&node->list);
+		sorted_n[i++] = node;
+	}
+	DBG_BUGON(i != shared_xattrs_count);
+	qsort(sorted_n, shared_xattrs_count, sizeof(n), comp_xattr_item);
+	for (i = 0; i < shared_xattrs_count; i++)
+		list_add_tail(&sorted_n[i]->list, &shared_xattrs_list);
+	free(sorted_n);
+
 	list_for_each_entry_safe(node, n, &shared_xattrs_list, list) {
 		struct xattr_item *const item = node->item;
 		const struct erofs_xattr_entry entry = {
-- 
2.25.1

[PATCH 2/2] erofs-utils: tests: add test for xattr

[Huang Jianan via Linux-erofs]

Add basic function check for xattr.

Signed-off-by: Huang Jianan <huangjianan@oppo.com>
---
 tests/Makefile.am   |  3 +++
 tests/erofs/019     | 63 +++++++++++++++++++++++++++++++++++++++++++++
 tests/erofs/019.out |  2 ++
 3 files changed, 68 insertions(+)
 create mode 100755 tests/erofs/019
 create mode 100644 tests/erofs/019.out

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6eeaece..da3e888 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -82,6 +82,9 @@ TESTS += erofs/017
 # 018 - verify lzma compressed image
 TESTS += erofs/018
 
+# 019 - check the xattr functionality
+TESTS += erofs/019
+
 EXTRA_DIST = common/rc erofs
 
 clean-local: clean-local-check
diff --git a/tests/erofs/019 b/tests/erofs/019
new file mode 100755
index 0000000..5e182a0
--- /dev/null
+++ b/tests/erofs/019
@@ -0,0 +1,63 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+
+# get standard environment, filters and checks
+. "${srcdir}/common/rc"
+
+cleanup()
+{
+	cd /
+	rm -rf $tmp.*
+}
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+echo "QA output created by $seq"
+
+_require_erofs
+
+[ -z "$lz4_on" ] && \
+	_notrun "lz4 compression is disabled, skipped."
+
+have_xattr=`which xattr`
+[ -z "$have_xattr" ] && \
+	_notrun "xattr isn't installed, skipped."
+
+if [ -z $SCRATCH_DEV ]; then
+	SCRATCH_DEV=$tmp/erofs_$seq.img
+	rm -f SCRATCH_DEV
+fi
+
+localdir="$tmp/$seq"
+rm -rf $localdir
+mkdir -p $localdir
+
+# set random xattr
+cp -nR ../ $localdir
+dirs=`ls $localdir`
+for d in $dirs; do
+	key=`head -20 /dev/urandom | cksum | cut -f1 -d " "`
+	val=`head -20 /dev/urandom | cksum | cut -f1 -d " "`
+	xattr -w user.$key $val $localdir/$d
+done
+
+_scratch_mkfs $localdir || _fail "failed to mkfs"
+
+# check xattr
+_scratch_mount 2>>$seqres.full
+
+for d in $dirs; do
+	xattr1=`xattr -l $localdir/$d`
+	xattr2=`xattr -l $SCRATCH_MNT/$d`
+	[ "$xattr1" = "$xattr2" ] || _fail "-->check xattr FAILED"
+done
+
+_scratch_unmount
+
+echo Silence is golden
+status=0
+exit 0
diff --git a/tests/erofs/019.out b/tests/erofs/019.out
new file mode 100644
index 0000000..163484b
--- /dev/null
+++ b/tests/erofs/019.out
@@ -0,0 +1,2 @@
+QA output created by 019
+Silence is golden
-- 
2.25.1

Re: [PATCH v2 1/2] erofs-utils: sort shared xattr

[Gao Xiang]

Hi Jianan,

On Tue, Dec 14, 2021 at 05:52:01PM +0800, Huang Jianan via Linux-erofs wrote:
> Sort shared xattr before writing to disk to ensure the consistency
> of reproducible builds.
> 
> Signed-off-by: Huang Jianan <huangjianan@oppo.com>

I still fail to understand why the order of shared xattrs will be
a problem of reproducible builds.

IOWs, if the order of shared xattrs is really a problem, do we need
to sort inline xattrs as well? Or do you have some reproducer to
show why the order of shared xattrs can be changed if the filesystem
of srcdir isn't changed?

Thanks,
Gao Xiang

Re: [PATCH v2 1/2] erofs-utils: sort shared xattr

[Huang Jianan via Linux-erofs]

Hi Xiang,

在 2021/12/14 23:31, Gao Xiang 写道:
> Hi Jianan,
>
> On Tue, Dec 14, 2021 at 05:52:01PM +0800, Huang Jianan via Linux-erofs wrote:
>> Sort shared xattr before writing to disk to ensure the consistency
>> of reproducible builds.
>>
>> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> I still fail to understand why the order of shared xattrs will be
> a problem of reproducible builds.
>
> IOWs, if the order of shared xattrs is really a problem, do we need
> to sort inline xattrs as well? Or do you have some reproducer to
> show why the order of shared xattrs can be changed if the filesystem
> of srcdir isn't changed?
We discovered this problem when we built the same image content on
different machines.

After executing readdir, the order of the subdirectories returned by the
two machines is different, which leads to a difference in the order of 
shared
xattr. I'm not sure if this scene can be called reproducible build in 
the strict
sense.

In addition, since the problem is caused by the readdir sequence, it should
have no effect on inline xattr.

Thanks,
Jianan
> Thanks,
> Gao Xiang
>

Re: [PATCH v2 1/2] erofs-utils: sort shared xattr

[Gao Xiang]

On Wed, Dec 15, 2021 at 10:43:40AM +0800, Huang Jianan via Linux-erofs wrote:
> Hi Xiang,
> 
> 在 2021/12/14 23:31, Gao Xiang 写道:
> > Hi Jianan,
> > 
> > On Tue, Dec 14, 2021 at 05:52:01PM +0800, Huang Jianan via Linux-erofs wrote:
> > > Sort shared xattr before writing to disk to ensure the consistency
> > > of reproducible builds.
> > > 
> > > Signed-off-by: Huang Jianan <huangjianan@oppo.com>
> > I still fail to understand why the order of shared xattrs will be
> > a problem of reproducible builds.
> > 
> > IOWs, if the order of shared xattrs is really a problem, do we need
> > to sort inline xattrs as well? Or do you have some reproducer to
> > show why the order of shared xattrs can be changed if the filesystem
> > of srcdir isn't changed?
> We discovered this problem when we built the same image content on
> different machines.
> 
> After executing readdir, the order of the subdirectories returned by the
> two machines is different, which leads to a difference in the order of
> shared
> xattr. I'm not sure if this scene can be called reproducible build in the
> strict
> sense.
> 
> In addition, since the problem is caused by the readdir sequence, it should
> have no effect on inline xattr.

So it sounds like we need to optimize xattr readdir process instead?

Thanks,
Gao Xiang

> 
> Thanks,
> Jianan
> > Thanks,
> > Gao Xiang
> > 

Re: [PATCH v2 1/2] erofs-utils: sort shared xattr

[Huang Jianan via Linux-erofs]

在 2021/12/15 11:22, Gao Xiang 写道:
> On Wed, Dec 15, 2021 at 10:43:40AM +0800, Huang Jianan via Linux-erofs wrote:
>> Hi Xiang,
>>
>> 在 2021/12/14 23:31, Gao Xiang 写道:
>>> Hi Jianan,
>>>
>>> On Tue, Dec 14, 2021 at 05:52:01PM +0800, Huang Jianan via Linux-erofs wrote:
>>>> Sort shared xattr before writing to disk to ensure the consistency
>>>> of reproducible builds.
>>>>
>>>> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
>>> I still fail to understand why the order of shared xattrs will be
>>> a problem of reproducible builds.
>>>
>>> IOWs, if the order of shared xattrs is really a problem, do we need
>>> to sort inline xattrs as well? Or do you have some reproducer to
>>> show why the order of shared xattrs can be changed if the filesystem
>>> of srcdir isn't changed?
>> We discovered this problem when we built the same image content on
>> different machines.
>>
>> After executing readdir, the order of the subdirectories returned by the
>> two machines is different, which leads to a difference in the order of
>> shared
>> xattr. I'm not sure if this scene can be called reproducible build in the
>> strict
>> sense.
>>
>> In addition, since the problem is caused by the readdir sequence, it should
>> have no effect on inline xattr.
> So it sounds like we need to optimize xattr readdir process instead?
Agree, currently xattr will prepare before sorting the subdirectories , 
I will try
to put the process after sorting.

Thanks,
Jianan
> Thanks,
> Gao Xiang
>
>> Thanks,
>> Jianan
>>> Thanks,
>>> Gao Xiang
>>>