[PATCH 0/7] (mainly) xz imports from Linux

[PATCH 0/7] (mainly) xz imports from Linux

[Jan Beulich]

While going through their 5.15.3 log I did notice two changes, which made
me go check what else we might be missing. The series here is the result.
Linux has also updated zstd, but that includes a pretty large change which
I'm not ready to deal with right now. Them moving closer to the upstream
zstd sources is certainly a good thing, so I suppose sooner or later we
will want to follow them in doing so.

1: xz: add fall-through comments to a switch statement
2: xz: fix XZ_DYNALLOC to avoid useless memory reallocations
3: decompressors: fix spelling mistakes
4: xz: avoid overlapping memcpy() with invalid input with in-place decompression
5: xz: fix spelling in comments
6: xz: move s->lzma.len = 0 initialization to lzma_reset()
7: xz: validate the value before assigning it to an enum variable

Jan

[PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

It's good style. I was also told that GCC 7 is more strict and might
give a warning when such comments are missing.

Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
[Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Linux has meanwhile further moved to using the "fallthrough" pseudo-
keyword, but us doing so requires the tool stack to first make this
available for use in at least stubdom builds.

--- a/xen/common/xz/dec_stream.c
+++ b/xen/common/xz/dec_stream.c
@@ -583,6 +583,8 @@ static enum xz_ret __init dec_main(struc
 			if (ret != XZ_OK)
 				return ret;
 
+		/* Fall through */
+
 		case SEQ_BLOCK_START:
 			/* We need one byte of input to continue. */
 			if (b->in_pos == b->in_size)
@@ -606,6 +608,8 @@ static enum xz_ret __init dec_main(struc
 			s->temp.pos = 0;
 			s->sequence = SEQ_BLOCK_HEADER;
 
+		/* Fall through */
+
 		case SEQ_BLOCK_HEADER:
 			if (!fill_temp(s, b))
 				return XZ_OK;
@@ -616,6 +620,8 @@ static enum xz_ret __init dec_main(struc
 
 			s->sequence = SEQ_BLOCK_UNCOMPRESS;
 
+		/* Fall through */
+
 		case SEQ_BLOCK_UNCOMPRESS:
 			ret = dec_block(s, b);
 			if (ret != XZ_STREAM_END)
@@ -623,6 +629,8 @@ static enum xz_ret __init dec_main(struc
 
 			s->sequence = SEQ_BLOCK_PADDING;
 
+		/* Fall through */
+
 		case SEQ_BLOCK_PADDING:
 			/*
 			 * Size of Compressed Data + Block Padding
@@ -643,6 +651,8 @@ static enum xz_ret __init dec_main(struc
 
 			s->sequence = SEQ_BLOCK_CHECK;
 
+		/* Fall through */
+
 		case SEQ_BLOCK_CHECK:
 			if (s->check_type == XZ_CHECK_CRC32) {
 				ret = crc32_validate(s, b);
@@ -665,6 +675,8 @@ static enum xz_ret __init dec_main(struc
 
 			s->sequence = SEQ_INDEX_PADDING;
 
+		/* Fall through */
+
 		case SEQ_INDEX_PADDING:
 			while ((s->index.size + (b->in_pos - s->in_start))
 					& 3) {
@@ -687,6 +699,8 @@ static enum xz_ret __init dec_main(struc
 
 			s->sequence = SEQ_INDEX_CRC32;
 
+		/* Fall through */
+
 		case SEQ_INDEX_CRC32:
 			ret = crc32_validate(s, b);
 			if (ret != XZ_STREAM_END)
@@ -695,6 +709,8 @@ static enum xz_ret __init dec_main(struc
 			s->temp.size = STREAM_HEADER_SIZE;
 			s->sequence = SEQ_STREAM_FOOTER;
 
+		/* Fall through */
+
 		case SEQ_STREAM_FOOTER:
 			if (!fill_temp(s, b))
 				return XZ_OK;

[PATCH 2/7] xz: fix XZ_DYNALLOC to avoid useless memory reallocations

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

s->dict.allocated was initialized to 0 but never set after a successful
allocation, thus the code always thought that the dictionary buffer has
to be reallocated.

Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
Reported-by: Yu Sun <yusun2@cisco.com>
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
Acked-by: Daniel Walker <danielwa@cisco.com>
[Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/xz/dec_lzma2.c
+++ b/xen/common/xz/dec_lzma2.c
@@ -1146,6 +1146,7 @@ XZ_EXTERN enum xz_ret __init xz_dec_lzma
 
 		if (DEC_IS_DYNALLOC(s->dict.mode)) {
 			if (s->dict.allocated < s->dict.size) {
+				s->dict.allocated = s->dict.size;
 				large_free(s->dict.buf);
 				s->dict.buf = large_malloc(s->dict.size);
 				if (s->dict.buf == NULL) {

[PATCH 3/7] decompressors: fix spelling mistakes

[Jan Beulich]

From: Zhen Lei <thunder.leizhen@huawei.com>

Fix some spelling mistakes in comments:
sentinal ==> sentinel
compresed ==> compressed
immediatelly ==> immediately
dervied ==> derived
splitted ==> split
nore ==> not
independed ==> independent
asumed ==> assumed

Link: https://lkml.kernel.org/r/20210604085656.12257-1-thunder.leizhen@huawei.com
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
[Linux commit: 05911c5d964956442d17fe21db239de5a1dace4a]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/bunzip2.c
+++ b/xen/common/bunzip2.c
@@ -73,7 +73,7 @@
 
 /* This is what we know about each Huffman coding group */
 struct group_data {
-	/* We have an extra slot at the end of limit[] for a sentinal value. */
+	/* We have an extra slot at the end of limit[] for a sentinel value. */
 	int limit[MAX_HUFCODE_BITS+1];
 	int base[MAX_HUFCODE_BITS];
 	int permute[MAX_SYMBOLS];
@@ -326,7 +326,7 @@ static int __init get_next_block(struct
 			pp <<= 1;
 			base[i+1] = pp-(t += temp[i]);
 		}
-		limit[maxLen+1] = INT_MAX; /* Sentinal value for
+		limit[maxLen+1] = INT_MAX; /* Sentinel value for
 					    * reading next sym. */
 		limit[maxLen] = pp+temp[maxLen]-1;
 		base[minLen] = 0;
--- a/xen/common/unxz.c
+++ b/xen/common/unxz.c
@@ -23,7 +23,7 @@
  * uncompressible. Thus, we must look for worst-case expansion when the
  * compressor is encoding uncompressible data.
  *
- * The structure of the .xz file in case of a compresed kernel is as follows.
+ * The structure of the .xz file in case of a compressed kernel is as follows.
  * Sizes (as bytes) of the fields are in parenthesis.
  *
  *    Stream Header (12)
--- a/xen/common/unzstd.c
+++ b/xen/common/unzstd.c
@@ -16,7 +16,7 @@
  * uncompressible. Thus, we must look for worst-case expansion when the
  * compressor is encoding uncompressible data.
  *
- * The structure of the .zst file in case of a compresed kernel is as follows.
+ * The structure of the .zst file in case of a compressed kernel is as follows.
  * Maximum sizes (as bytes) of the fields are in parenthesis.
  *
  *    Frame Header: (18)
--- a/xen/common/xz/dec_bcj.c
+++ b/xen/common/xz/dec_bcj.c
@@ -422,7 +422,7 @@ XZ_EXTERN enum xz_ret __init xz_dec_bcj_
 
 	/*
 	 * Flush pending already filtered data to the output buffer. Return
-	 * immediatelly if we couldn't flush everything, or if the next
+	 * immediately if we couldn't flush everything, or if the next
 	 * filter in the chain had already returned XZ_STREAM_END.
 	 */
 	if (s->temp.filtered > 0) {
--- a/xen/common/xz/dec_lzma2.c
+++ b/xen/common/xz/dec_lzma2.c
@@ -147,8 +147,8 @@ struct lzma_dec {
 
 	/*
 	 * LZMA properties or related bit masks (number of literal
-	 * context bits, a mask dervied from the number of literal
-	 * position bits, and a mask dervied from the number
+	 * context bits, a mask derived from the number of literal
+	 * position bits, and a mask derived from the number
 	 * position bits)
 	 */
 	uint32_t lc;
@@ -484,7 +484,7 @@ static always_inline void rc_normalize(s
 }
 
 /*
- * Decode one bit. In some versions, this function has been splitted in three
+ * Decode one bit. In some versions, this function has been split in three
  * functions so that the compiler is supposed to be able to more easily avoid
  * an extra branch. In this particular version of the LZMA decoder, this
  * doesn't seem to be a good idea (tested with GCC 3.3.6, 3.4.6, and 4.3.3
@@ -761,7 +761,7 @@ static bool_t __init lzma_main(struct xz
 }
 
 /*
- * Reset the LZMA decoder and range decoder state. Dictionary is nore reset
+ * Reset the LZMA decoder and range decoder state. Dictionary is not reset
  * here, because LZMA state may be reset without resetting the dictionary.
  */
 static void __init lzma_reset(struct xz_dec_lzma2 *s)
--- a/xen/common/zstd/huf.h
+++ b/xen/common/zstd/huf.h
@@ -131,7 +131,7 @@ typedef enum {
 	HUF_repeat_none,  /**< Cannot use the previous table */
 	HUF_repeat_check, /**< Can use the previous table but it must be checked. Note : The previous table must have been constructed by HUF_compress{1,
 			     4}X_repeat */
-	HUF_repeat_valid  /**< Can use the previous table and it is asumed to be valid */
+	HUF_repeat_valid  /**< Can use the previous table and it is assumed to be valid */
 } HUF_repeat;
 /** HUF_compress4X_repeat() :
 *   Same as HUF_compress4X_wksp(), but considers using hufTable if *repeat != HUF_repeat_none.

[PATCH 4/7] xz: avoid overlapping memcpy() with invalid input with in-place decompression

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

With valid files, the safety margin described in lib/decompress_unxz.c
ensures that these buffers cannot overlap. But if the uncompressed size
of the input is larger than the caller thought, which is possible when
the input file is invalid/corrupt, the buffers can overlap. Obviously
the result will then be garbage (and usually the decoder will return
an error too) but no other harm will happen when such an over-run occurs.

This change only affects uncompressed LZMA2 chunks and so this
should have no effect on performance.

Link: https://lore.kernel.org/r/20211010213145.17462-2-xiang@kernel.org
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
[Linux commit: 83d3c4f22a36d005b55f44628f46cc0d319a75e8]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/unxz.c
+++ b/xen/common/unxz.c
@@ -127,7 +127,7 @@
  * memeq and memzero are not used much and any remotely sane implementation
  * is fast enough. memcpy/memmove speed matters in multi-call mode, but
  * the kernel image is decompressed in single-call mode, in which only
- * memcpy speed can matter and only if there is a lot of uncompressible data
+ * memmove speed can matter and only if there is a lot of uncompressible data
  * (LZMA2 stores uncompressible chunks in uncompressed form). Thus, the
  * functions below should just be kept small; it's probably not worth
  * optimizing for speed.
--- a/xen/common/xz/dec_lzma2.c
+++ b/xen/common/xz/dec_lzma2.c
@@ -387,7 +387,14 @@ static void __init dict_uncompressed(str
 
 		*left -= copy_size;
 
-		memcpy(dict->buf + dict->pos, b->in + b->in_pos, copy_size);
+		/*
+		 * If doing in-place decompression in single-call mode and the
+		 * uncompressed size of the file is larger than the caller
+		 * thought (i.e. it is invalid input!), the buffers below may
+		 * overlap and cause undefined behavior with memcpy().
+		 * With valid inputs memcpy() would be fine here.
+		 */
+		memmove(dict->buf + dict->pos, b->in + b->in_pos, copy_size);
 		dict->pos += copy_size;
 
 		if (dict->full < dict->pos)
@@ -397,7 +404,11 @@ static void __init dict_uncompressed(str
 			if (dict->pos == dict->end)
 				dict->pos = 0;
 
-			memcpy(b->out + b->out_pos, b->in + b->in_pos,
+			/*
+			 * Like above but for multi-call mode: use memmove()
+			 * to avoid undefined behavior with invalid input.
+			 */
+			memmove(b->out + b->out_pos, b->in + b->in_pos,
 					copy_size);
 		}
 
@@ -421,6 +432,12 @@ static uint32_t __init dict_flush(struct
 		if (dict->pos == dict->end)
 			dict->pos = 0;
 
+		/*
+		 * These buffers cannot overlap even if doing in-place
+		 * decompression because in multi-call mode dict->buf
+		 * has been allocated by us in this file; it's not
+		 * provided by the caller like in single-call mode.
+		 */
 		memcpy(b->out + b->out_pos, dict->buf + dict->start,
 				copy_size);
 	}

[PATCH 5/7] xz: fix spelling in comments

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

uncompressible -> incompressible
non-splitted -> non-split

Link: https://lore.kernel.org/r/20211010213145.17462-6-xiang@kernel.org
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
[Linux commit: 0a434e0a2c9f4395e4560aac22677ef25ab4afd9]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/unxz.c
+++ b/xen/common/unxz.c
@@ -20,8 +20,8 @@
  *
  * The worst case for in-place decompression is that the beginning of
  * the file is compressed extremely well, and the rest of the file is
- * uncompressible. Thus, we must look for worst-case expansion when the
- * compressor is encoding uncompressible data.
+ * incompressible. Thus, we must look for worst-case expansion when the
+ * compressor is encoding incompressible data.
  *
  * The structure of the .xz file in case of a compressed kernel is as follows.
  * Sizes (as bytes) of the fields are in parenthesis.
@@ -58,7 +58,7 @@
  * uncompressed size of the payload is in practice never less than the
  * payload size itself. The LZMA2 format would allow uncompressed size
  * to be less than the payload size, but no sane compressor creates such
- * files. LZMA2 supports storing uncompressible data in uncompressed form,
+ * files. LZMA2 supports storing incompressible data in uncompressed form,
  * so there's never a need to create payloads whose uncompressed size is
  * smaller than the compressed size.
  *
@@ -127,8 +127,8 @@
  * memeq and memzero are not used much and any remotely sane implementation
  * is fast enough. memcpy/memmove speed matters in multi-call mode, but
  * the kernel image is decompressed in single-call mode, in which only
- * memmove speed can matter and only if there is a lot of uncompressible data
- * (LZMA2 stores uncompressible chunks in uncompressed form). Thus, the
+ * memmove speed can matter and only if there is a lot of incompressible data
+ * (LZMA2 stores incompressible chunks in uncompressed form). Thus, the
  * functions below should just be kept small; it's probably not worth
  * optimizing for speed.
  */
--- a/xen/common/xz/dec_lzma2.c
+++ b/xen/common/xz/dec_lzma2.c
@@ -505,7 +505,7 @@ static always_inline void rc_normalize(s
  * functions so that the compiler is supposed to be able to more easily avoid
  * an extra branch. In this particular version of the LZMA decoder, this
  * doesn't seem to be a good idea (tested with GCC 3.3.6, 3.4.6, and 4.3.3
- * on x86). Using a non-splitted version results in nicer looking code too.
+ * on x86). Using a non-split version results in nicer looking code too.
  *
  * NOTE: This must return an int. Do not make it return a bool or the speed
  * of the code generated by GCC 3.x decreases 10-15 %. (GCC 4.3 doesn't care,

[PATCH 6/7] xz: move s->lzma.len = 0 initialization to lzma_reset()

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

It's a more logical place even if the resetting needs to be done
only once per LZMA2 stream (if lzma_reset() called in the middle
of an LZMA2 stream, .len will already be 0).

Link: https://lore.kernel.org/r/20211010213145.17462-4-xiang@kernel.org
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
[Linux commit: a98a25408b0e9b0264abcc3dabfafd9ff2ea1046]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/xz/dec_lzma2.c
+++ b/xen/common/xz/dec_lzma2.c
@@ -791,6 +791,7 @@ static void __init lzma_reset(struct xz_
 	s->lzma.rep1 = 0;
 	s->lzma.rep2 = 0;
 	s->lzma.rep3 = 0;
+	s->lzma.len = 0;
 
 	/*
 	 * All probabilities are initialized to the same value. This hack
@@ -1174,8 +1175,6 @@ XZ_EXTERN enum xz_ret __init xz_dec_lzma
 		}
 	}
 
-	s->lzma.len = 0;
-
 	s->lzma2.sequence = SEQ_CONTROL;
 	s->lzma2.need_dict_reset = true;
 

[PATCH 7/7] xz: validate the value before assigning it to an enum variable

[Jan Beulich]

From: Lasse Collin <lasse.collin@tukaani.org>

This might matter, for example, if the underlying type of enum xz_check
was a signed char. In such a case the validation wouldn't have caught an
unsupported header. I don't know if this problem can occur in the kernel
on any arch but it's still good to fix it because some people might copy
the XZ code to their own projects from Linux instead of the upstream
XZ Embedded repository.

This change may increase the code size by a few bytes. An alternative
would have been to use an unsigned int instead of enum xz_check but
using an enumeration looks cleaner.

Link: https://lore.kernel.org/r/20211010213145.17462-3-xiang@kernel.org
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
[Linux commit: 4f8d7abaa413c34da9d751289849dbfb7c977d05]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/xz/dec_stream.c
+++ b/xen/common/xz/dec_stream.c
@@ -402,12 +402,12 @@ static enum xz_ret __init dec_stream_hea
 	 * we will accept other check types too, but then the check won't
 	 * be verified and a warning (XZ_UNSUPPORTED_CHECK) will be given.
 	 */
+	if (s->temp.buf[HEADER_MAGIC_SIZE + 1] > XZ_CHECK_MAX)
+		return XZ_OPTIONS_ERROR;
+
 	s->check_type = s->temp.buf[HEADER_MAGIC_SIZE + 1];
 
 #ifdef XZ_DEC_ANY_CHECK
-	if (s->check_type > XZ_CHECK_MAX)
-		return XZ_OPTIONS_ERROR;
-
 	if (s->check_type > XZ_CHECK_CRC32)
 		return XZ_UNSUPPORTED_CHECK;
 #else

Re: [PATCH 0/7] (mainly) xz imports from Linux

[Ian Jackson]

Jan Beulich writes ("[PATCH 0/7] (mainly) xz imports from Linux"):
> While going through their 5.15.3 log I did notice two changes, which made
> me go check what else we might be missing. The series here is the result.
> Linux has also updated zstd, but that includes a pretty large change which
> I'm not ready to deal with right now. Them moving closer to the upstream
> zstd sources is certainly a good thing, so I suppose sooner or later we
> will want to follow them in doing so.
> 
> 1: xz: add fall-through comments to a switch statement
> 2: xz: fix XZ_DYNALLOC to avoid useless memory reallocations
> 3: decompressors: fix spelling mistakes
> 4: xz: avoid overlapping memcpy() with invalid input with in-place decompression
> 5: xz: fix spelling in comments
> 6: xz: move s->lzma.len = 0 initialization to lzma_reset()
> 7: xz: validate the value before assigning it to an enum variable

FTAOD I think none of these are critical bug fixes for 4.16.
Please let me know if I'm wrong.

In theory 4 is UB but in practice the result is presumably just wrong
answers.

Ian.

Re: [PATCH 0/7] (mainly) xz imports from Linux

[Jan Beulich]

On 19.11.2021 15:25, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 0/7] (mainly) xz imports from Linux"):
>> While going through their 5.15.3 log I did notice two changes, which made
>> me go check what else we might be missing. The series here is the result.
>> Linux has also updated zstd, but that includes a pretty large change which
>> I'm not ready to deal with right now. Them moving closer to the upstream
>> zstd sources is certainly a good thing, so I suppose sooner or later we
>> will want to follow them in doing so.
>>
>> 1: xz: add fall-through comments to a switch statement
>> 2: xz: fix XZ_DYNALLOC to avoid useless memory reallocations
>> 3: decompressors: fix spelling mistakes
>> 4: xz: avoid overlapping memcpy() with invalid input with in-place decompression
>> 5: xz: fix spelling in comments
>> 6: xz: move s->lzma.len = 0 initialization to lzma_reset()
>> 7: xz: validate the value before assigning it to an enum variable
> 
> FTAOD I think none of these are critical bug fixes for 4.16.
> Please let me know if I'm wrong.

Indeed, you're not wrong, and I intentionally didn't tag them that way. All
I wanted is to get them out rather than sit on them.

> In theory 4 is UB but in practice the result is presumably just wrong
> answers.

Like Linux did, the plan is to backport that and perhaps 7. But there's no
urgency.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi Jan,

On 19/11/2021 10:21, Jan Beulich wrote:
> From: Lasse Collin <lasse.collin@tukaani.org>
> 
> It's good style. I was also told that GCC 7 is more strict and might
> give a warning when such comments are missing.
> 
> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> [Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
> Linux has meanwhile further moved to using the "fallthrough" pseudo-
> keyword, but us doing so requires the tool stack to first make this
> available for use in at least stubdom builds.
> 
> --- a/xen/common/xz/dec_stream.c
> +++ b/xen/common/xz/dec_stream.c
> @@ -583,6 +583,8 @@ static enum xz_ret __init dec_main(struc
>   			if (ret != XZ_OK)
>   				return ret;
>   
> +		/* Fall through */
> +
>   		case SEQ_BLOCK_START:
>   			/* We need one byte of input to continue. */
>   			if (b->in_pos == b->in_size)
> @@ -606,6 +608,8 @@ static enum xz_ret __init dec_main(struc
>   			s->temp.pos = 0;
>   			s->sequence = SEQ_BLOCK_HEADER;
>   
> +		/* Fall through */
> +
>   		case SEQ_BLOCK_HEADER:
>   			if (!fill_temp(s, b))
>   				return XZ_OK;
> @@ -616,6 +620,8 @@ static enum xz_ret __init dec_main(struc
>   
>   			s->sequence = SEQ_BLOCK_UNCOMPRESS;
>   
> +		/* Fall through */
> +
>   		case SEQ_BLOCK_UNCOMPRESS:
>   			ret = dec_block(s, b);
>   			if (ret != XZ_STREAM_END)
> @@ -623,6 +629,8 @@ static enum xz_ret __init dec_main(struc
>   
>   			s->sequence = SEQ_BLOCK_PADDING;
>   
> +		/* Fall through */
> +
>   		case SEQ_BLOCK_PADDING:
>   			/*
>   			 * Size of Compressed Data + Block Padding
> @@ -643,6 +651,8 @@ static enum xz_ret __init dec_main(struc
>   
>   			s->sequence = SEQ_BLOCK_CHECK;
>   
> +		/* Fall through */
> +
>   		case SEQ_BLOCK_CHECK:
>   			if (s->check_type == XZ_CHECK_CRC32) {
>   				ret = crc32_validate(s, b);
> @@ -665,6 +675,8 @@ static enum xz_ret __init dec_main(struc
>   
>   			s->sequence = SEQ_INDEX_PADDING;
>   
> +		/* Fall through */
> +
>   		case SEQ_INDEX_PADDING:
>   			while ((s->index.size + (b->in_pos - s->in_start))
>   					& 3) {
> @@ -687,6 +699,8 @@ static enum xz_ret __init dec_main(struc
>   
>   			s->sequence = SEQ_INDEX_CRC32;
>   
> +		/* Fall through */
> +
>   		case SEQ_INDEX_CRC32:
>   			ret = crc32_validate(s, b);
>   			if (ret != XZ_STREAM_END)
> @@ -695,6 +709,8 @@ static enum xz_ret __init dec_main(struc
>   			s->temp.size = STREAM_HEADER_SIZE;
>   			s->sequence = SEQ_STREAM_FOOTER;
>   
> +		/* Fall through */
> +
>   		case SEQ_STREAM_FOOTER:
>   			if (!fill_temp(s, b))
>   				return XZ_OK;
> 

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

On 25/11/2021 16:49, Julien Grall wrote:
> On 19/11/2021 10:21, Jan Beulich wrote:
>> From: Lasse Collin <lasse.collin@tukaani.org>
>>
>> It's good style. I was also told that GCC 7 is more strict and might
>> give a warning when such comments are missing.
>>
>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>

Actually, any reason why there are some signed-off-by missing?

>> [Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
>> ---
>> Linux has meanwhile further moved to using the "fallthrough" pseudo-
>> keyword, but us doing so requires the tool stack to first make this
>> available for use in at least stubdom builds.
>>
>> --- a/xen/common/xz/dec_stream.c
>> +++ b/xen/common/xz/dec_stream.c
>> @@ -583,6 +583,8 @@ static enum xz_ret __init dec_main(struc
>>               if (ret != XZ_OK)
>>                   return ret;
>> +        /* Fall through */
>> +
>>           case SEQ_BLOCK_START:
>>               /* We need one byte of input to continue. */
>>               if (b->in_pos == b->in_size)
>> @@ -606,6 +608,8 @@ static enum xz_ret __init dec_main(struc
>>               s->temp.pos = 0;
>>               s->sequence = SEQ_BLOCK_HEADER;
>> +        /* Fall through */
>> +
>>           case SEQ_BLOCK_HEADER:
>>               if (!fill_temp(s, b))
>>                   return XZ_OK;
>> @@ -616,6 +620,8 @@ static enum xz_ret __init dec_main(struc
>>               s->sequence = SEQ_BLOCK_UNCOMPRESS;
>> +        /* Fall through */
>> +
>>           case SEQ_BLOCK_UNCOMPRESS:
>>               ret = dec_block(s, b);
>>               if (ret != XZ_STREAM_END)
>> @@ -623,6 +629,8 @@ static enum xz_ret __init dec_main(struc
>>               s->sequence = SEQ_BLOCK_PADDING;
>> +        /* Fall through */
>> +
>>           case SEQ_BLOCK_PADDING:
>>               /*
>>                * Size of Compressed Data + Block Padding
>> @@ -643,6 +651,8 @@ static enum xz_ret __init dec_main(struc
>>               s->sequence = SEQ_BLOCK_CHECK;
>> +        /* Fall through */
>> +
>>           case SEQ_BLOCK_CHECK:
>>               if (s->check_type == XZ_CHECK_CRC32) {
>>                   ret = crc32_validate(s, b);
>> @@ -665,6 +675,8 @@ static enum xz_ret __init dec_main(struc
>>               s->sequence = SEQ_INDEX_PADDING;
>> +        /* Fall through */
>> +
>>           case SEQ_INDEX_PADDING:
>>               while ((s->index.size + (b->in_pos - s->in_start))
>>                       & 3) {
>> @@ -687,6 +699,8 @@ static enum xz_ret __init dec_main(struc
>>               s->sequence = SEQ_INDEX_CRC32;
>> +        /* Fall through */
>> +
>>           case SEQ_INDEX_CRC32:
>>               ret = crc32_validate(s, b);
>>               if (ret != XZ_STREAM_END)
>> @@ -695,6 +709,8 @@ static enum xz_ret __init dec_main(struc
>>               s->temp.size = STREAM_HEADER_SIZE;
>>               s->sequence = SEQ_STREAM_FOOTER;
>> +        /* Fall through */
>> +
>>           case SEQ_STREAM_FOOTER:
>>               if (!fill_temp(s, b))
>>                   return XZ_OK;
>>
> 

-- 
Julien Grall

Re: [PATCH 2/7] xz: fix XZ_DYNALLOC to avoid useless memory reallocations

[Julien Grall]

Hi Jan,

On 19/11/2021 10:21, Jan Beulich wrote:
> From: Lasse Collin <lasse.collin@tukaani.org>
> 
> s->dict.allocated was initialized to 0 but never set after a successful
> allocation, thus the code always thought that the dictionary buffer has
> to be reallocated.
> 
> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
> Reported-by: Yu Sun <yusun2@cisco.com>
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> Acked-by: Daniel Walker <danielwa@cisco.com>
> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]

This commit contains two more signed-off-by. Any reason to not have 
included them here?

The rest of the patch LGTM.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/xz/dec_lzma2.c
> +++ b/xen/common/xz/dec_lzma2.c
> @@ -1146,6 +1146,7 @@ XZ_EXTERN enum xz_ret __init xz_dec_lzma
>   
>   		if (DEC_IS_DYNALLOC(s->dict.mode)) {
>   			if (s->dict.allocated < s->dict.size) {
> +				s->dict.allocated = s->dict.size;
>   				large_free(s->dict.buf);
>   				s->dict.buf = large_malloc(s->dict.size);
>   				if (s->dict.buf == NULL) {
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] decompressors: fix spelling mistakes

[Julien Grall]

Hi Jan,

On 19/11/2021 10:21, Jan Beulich wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Fix some spelling mistakes in comments:
> sentinal ==> sentinel
> compresed ==> compressed
> immediatelly ==> immediately
> dervied ==> derived
> splitted ==> split
> nore ==> not
> independed ==> independent
> asumed ==> assumed
> 
> Link: https://lkml.kernel.org/r/20210604085656.12257-1-thunder.leizhen@huawei.com
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> [Linux commit: 05911c5d964956442d17fe21db239de5a1dace4a]

Same question as the two previous patch for the missing signed-off-by. 
The rest looks fine to me.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 25.11.2021 17:54, Julien Grall wrote:
> On 25/11/2021 16:49, Julien Grall wrote:
>> On 19/11/2021 10:21, Jan Beulich wrote:
>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>
>>> It's good style. I was also told that GCC 7 is more strict and might
>>> give a warning when such comments are missing.
>>>
>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> 
> Actually, any reason why there are some signed-off-by missing?

I often keep the author's, but drop ones which clearly got there only
because of the path a patch has taken through trees. These aren't
relevant imo when pulling over the change; I could as well take the
email submission as my basis, after all, where just the single S-o-b
would be there.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi,

On 25/11/2021 17:03, Jan Beulich wrote:
> On 25.11.2021 17:54, Julien Grall wrote:
>> On 25/11/2021 16:49, Julien Grall wrote:
>>> On 19/11/2021 10:21, Jan Beulich wrote:
>>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>>
>>>> It's good style. I was also told that GCC 7 is more strict and might
>>>> give a warning when such comments are missing.
>>>>
>>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>
>> Actually, any reason why there are some signed-off-by missing?
> 
> I often keep the author's, but drop ones which clearly got there only
> because of the path a patch has taken through trees.

This might be clear for you. For me, as a reviewer, I have to do extra 
work to check whether you keeped the relevant signed-off-by.

> These aren't
> relevant imo when pulling over the change;

They are technically part of the "chain of approval".

> I could as well take the
> email submission as my basis, after all, where just the single S-o-b
> would be there.

That's a fair point. That said, you took the commit-as-is from linus.git 
so I think we ought to keep them.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 25.11.2021 18:13, Julien Grall wrote:
> Hi,
> 
> On 25/11/2021 17:03, Jan Beulich wrote:
>> On 25.11.2021 17:54, Julien Grall wrote:
>>> On 25/11/2021 16:49, Julien Grall wrote:
>>>> On 19/11/2021 10:21, Jan Beulich wrote:
>>>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>>>
>>>>> It's good style. I was also told that GCC 7 is more strict and might
>>>>> give a warning when such comments are missing.
>>>>>
>>>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>>
>>> Actually, any reason why there are some signed-off-by missing?
>>
>> I often keep the author's, but drop ones which clearly got there only
>> because of the path a patch has taken through trees.
> 
> This might be clear for you. For me, as a reviewer, I have to do extra 
> work to check whether you keeped the relevant signed-off-by.
> 
>> These aren't
>> relevant imo when pulling over the change;
> 
> They are technically part of the "chain of approval".

But the Linux chain of approval is precisely what is of no interest to
us. We need to approve the change ourselves; Linux having had it
approved is merely a data point.

>> I could as well take the
>> email submission as my basis, after all, where just the single S-o-b
>> would be there.
> 
> That's a fair point. That said, you took the commit-as-is from linus.git 

How would you be able to tell?

> so I think we ought to keep them.

I disagree. And I'd like to remain consistent with what I've been doing
in the past.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi Jan,

On 26/11/2021 07:37, Jan Beulich wrote:
> On 25.11.2021 18:13, Julien Grall wrote:
>> Hi,
>>
>> On 25/11/2021 17:03, Jan Beulich wrote:
>>> On 25.11.2021 17:54, Julien Grall wrote:
>>>> On 25/11/2021 16:49, Julien Grall wrote:
>>>>> On 19/11/2021 10:21, Jan Beulich wrote:
>>>>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>>>>
>>>>>> It's good style. I was also told that GCC 7 is more strict and might
>>>>>> give a warning when such comments are missing.
>>>>>>
>>>>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>>>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>>>
>>>> Actually, any reason why there are some signed-off-by missing?
>>>
>>> I often keep the author's, but drop ones which clearly got there only
>>> because of the path a patch has taken through trees.
>>
>> This might be clear for you. For me, as a reviewer, I have to do extra
>> work to check whether you keeped the relevant signed-off-by.
>>
>>> These aren't
>>> relevant imo when pulling over the change;
>>
>> They are technically part of the "chain of approval".
> 
> But the Linux chain of approval is precisely what is of no interest to
> us. We need to approve the change ourselves; Linux having had it
> approved is merely a data point.

I can understand this point of view. But as I wrote above, a reviewer as 
to do extra work to check you correctly propagated the signed-off-by 
(see more below).

> 
>>> I could as well take the
>>> email submission as my basis, after all, where just the single S-o-b
>>> would be there.
>>
>> That's a fair point. That said, you took the commit-as-is from linus.git
> 
> How would you be able to tell?

That's easy. You wrote in your commit message:

[Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]

That's indicating you used the Linux commit rather than the one on the 
ML. So I will tend to diff the commit and the what's different.

> 
>> so I think we ought to keep them.
> 
> I disagree. And I'd like to remain consistent with what I've been doing
> in the past.
I actually tried to find the original submission but failed. I didn't 
look for long though...

Anyway, I think it would save time for everyone (you had to manually 
delete signed-off-by after all) if you just copy the commit (including 
all the signed-off-by) message as-is.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 26.11.2021 10:03, Julien Grall wrote:
> On 26/11/2021 07:37, Jan Beulich wrote:
>> On 25.11.2021 18:13, Julien Grall wrote:
>>> On 25/11/2021 17:03, Jan Beulich wrote:
>>>> On 25.11.2021 17:54, Julien Grall wrote:
>>>>> On 25/11/2021 16:49, Julien Grall wrote:
>>>>>> On 19/11/2021 10:21, Jan Beulich wrote:
>>>>>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>>>>>
>>>>>>> It's good style. I was also told that GCC 7 is more strict and might
>>>>>>> give a warning when such comments are missing.
>>>>>>>
>>>>>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>>>>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>>>>
>>>>> Actually, any reason why there are some signed-off-by missing?
>>>>
>>>> I often keep the author's, but drop ones which clearly got there only
>>>> because of the path a patch has taken through trees.
>>>
>>> This might be clear for you. For me, as a reviewer, I have to do extra
>>> work to check whether you keeped the relevant signed-off-by.
>>>
>>>> These aren't
>>>> relevant imo when pulling over the change;
>>>
>>> They are technically part of the "chain of approval".
>>
>> But the Linux chain of approval is precisely what is of no interest to
>> us. We need to approve the change ourselves; Linux having had it
>> approved is merely a data point.
> 
> I can understand this point of view. But as I wrote above, a reviewer as 
> to do extra work to check you correctly propagated the signed-off-by 
> (see more below).
> 
>>
>>>> I could as well take the
>>>> email submission as my basis, after all, where just the single S-o-b
>>>> would be there.
>>>
>>> That's a fair point. That said, you took the commit-as-is from linus.git
>>
>> How would you be able to tell?
> 
> That's easy. You wrote in your commit message:
> 
> [Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]
> 
> That's indicating you used the Linux commit rather than the one on the 
> ML. So I will tend to diff the commit and the what's different.

I don't view this as such an indication. I could have taken the submission
and merely have looked up the corresponding commit to provide a reference.

I think our re-using of Linux submissions should be indistinguishable from
their authors, if they were aware of and cared about our cloned code,
submitting their changes separately to xen-devel.

> Anyway, I think it would save time for everyone (you had to manually 
> delete signed-off-by after all) if you just copy the commit (including 
> all the signed-off-by) message as-is.

I don't think I see why you found it necessary to verify the S-o-b set.

Also note that, for things to be useful in our tree, I may also edit
commit messages in mechanical ways (e.g. to change file or function
names). I don't think you can expect a 1:1 match in any event. Review
of such submissions would normally mainly mean making sure that
everything was transformed correctly (besides the question whether the
patch is applicable to us in the first place), not that everything
matches up directly.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi Jan,

On 26/11/2021 09:12, Jan Beulich wrote:
>> Anyway, I think it would save time for everyone (you had to manually
>> delete signed-off-by after all) if you just copy the commit (including
>> all the signed-off-by) message as-is.
> 
> I don't think I see why you found it necessary to verify the S-o-b set.

This is a list of difference with the Linux commit that was unexplained 
to me.

> 
> Also note that, for things to be useful in our tree, I may also edit
> commit messages in mechanical ways (e.g. to change file or function
> names). I don't think you can expect a 1:1 match in any event.

I am fully aware that I can't expect a 1:1 match. However, if I see a 
difference, then I need to be able to explain it.

For this case, you provided some sort of an explanation but so far, I am 
still waiting for a link to confirm that the signed-off-by match the one 
on the ML.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 26.11.2021 11:04, Julien Grall wrote:
> Hi Jan,
> 
> On 26/11/2021 09:12, Jan Beulich wrote:
>>> Anyway, I think it would save time for everyone (you had to manually
>>> delete signed-off-by after all) if you just copy the commit (including
>>> all the signed-off-by) message as-is.
>>
>> I don't think I see why you found it necessary to verify the S-o-b set.
> 
> This is a list of difference with the Linux commit that was unexplained 
> to me.
> 
>>
>> Also note that, for things to be useful in our tree, I may also edit
>> commit messages in mechanical ways (e.g. to change file or function
>> names). I don't think you can expect a 1:1 match in any event.
> 
> I am fully aware that I can't expect a 1:1 match. However, if I see a 
> difference, then I need to be able to explain it.
> 
> For this case, you provided some sort of an explanation but so far, I am 
> still waiting for a link to confirm that the signed-off-by match the one 
> on the ML.

I haven't been able to easily find a mail archive holding this patch.
However, to me

http://lkml.iu.edu/hypermail/linux/kernel/1710.1/04375.html

clearly suggests that Jiri merely took the patch and applied it.

For patches 2 and onwards the Linux commits contain links (which I
did also retain in the posted patches), i.e.

https://lore.kernel.org/all/20191104185107.3b6330df@tukaani.org/T/#u
https://lore.kernel.org/all/20210604085656.12257-1-thunder.leizhen@huawei.com/T/#u
https://lore.kernel.org/all/20211010213145.17462-2-xiang@kernel.org/
https://lore.kernel.org/all/20211010213145.17462-6-xiang@kernel.org/
https://lore.kernel.org/all/20211010213145.17462-4-xiang@kernel.org/
https://lore.kernel.org/all/20211010213145.17462-3-xiang@kernel.org/

Going through those made me notice that on patches 4 and onwards I
should put back one more S-o-b, albeit for all four I then can't help
thinking that authorship is really the other way around. But I'm not
going to put effort into finding out ...

This exercise also made me notice that I have the last three patches
the wrong way round. Not that this would matter much.

FTAOD: Are these further inquiries of yours actually intended to tell
me that I should not have applied the ack that you've sent first for
this one patch?

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
> On 26.11.2021 11:04, Julien Grall wrote:
> > For this case, you provided some sort of an explanation but so far, I am 
> > still waiting for a link to confirm that the signed-off-by match the one 
> > on the ML.
> 
> I haven't been able to easily find a mail archive holding this patch.

I 100% agree with Julien on all points in this thread.

Please can we keep the Linux S-o-b.

Note that S-o-b is not a chain of *approval* (whose relevance to us is
debateable) but but a chain of *custody and transmission* for
copyright/licence/gdpr purposes.  That latter chain is hightly
relevant to us.

All such S-o-b should be retained.

Of course if you got the patch somewhere other than the Linux commit,
then the chain of custody doesn't pass through the Linux commit.  But
in that case I expect you to be able to say where you got it.

Instead, I suggest that, having established what the Linux commit is,
it is far simpler for us all to regard the Linux commit as our
immediate source for the patch, rather than some ML post which you are
not, apparently, able to find.

Ian.

Re: [PATCH 0/7] (mainly) xz imports from Linux

[Luca Fancellu]

Hi Jan,

> On 19 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
> 
> While going through their 5.15.3 log I did notice two changes, which made
> me go check what else we might be missing. The series here is the result.
> Linux has also updated zstd, but that includes a pretty large change which
> I'm not ready to deal with right now. Them moving closer to the upstream
> zstd sources is certainly a good thing, so I suppose sooner or later we
> will want to follow them in doing so.
> 
> 1: xz: add fall-through comments to a switch statement
> 2: xz: fix XZ_DYNALLOC to avoid useless memory reallocations
> 3: decompressors: fix spelling mistakes
> 4: xz: avoid overlapping memcpy() with invalid input with in-place decompression
> 5: xz: fix spelling in comments
> 6: xz: move s->lzma.len = 0 initialization to lzma_reset()
> 7: xz: validate the value before assigning it to an enum variable
> 

For the whole serie

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 26.11.2021 13:52, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>> On 26.11.2021 11:04, Julien Grall wrote:
>>> For this case, you provided some sort of an explanation but so far, I am 
>>> still waiting for a link to confirm that the signed-off-by match the one 
>>> on the ML.
>>
>> I haven't been able to easily find a mail archive holding this patch.
> 
> I 100% agree with Julien on all points in this thread.
> 
> Please can we keep the Linux S-o-b.
> 
> Note that S-o-b is not a chain of *approval* (whose relevance to us is
> debateable) but but a chain of *custody and transmission* for
> copyright/licence/gdpr purposes.  That latter chain is hightly
> relevant to us.
> 
> All such S-o-b should be retained.
> 
> Of course if you got the patch somewhere other than the Linux commit,
> then the chain of custody doesn't pass through the Linux commit.  But
> in that case I expect you to be able to say where you got it.

I've submitted v2 with S-o-b restored as far as necessary to meet this
requirement. I did not restore all of them, because I continue to not
see the value of retaining them. You saying "is highly relevant to us"
is a statement, but not an explanation of why tags beyond those in the
original submissions need retaining.

Without me seeing the need / value, I'm afraid I see only two ways
forward: Either things are acceptable as they are now (and will be for
future similar imports), or it needs to be someone else to put time
into spotting and then pulling in such changes.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi Jan,

On 06/12/2021 13:44, Jan Beulich wrote:
> On 26.11.2021 13:52, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>>> On 26.11.2021 11:04, Julien Grall wrote:
>>>> For this case, you provided some sort of an explanation but so far, I am
>>>> still waiting for a link to confirm that the signed-off-by match the one
>>>> on the ML.
>>>
>>> I haven't been able to easily find a mail archive holding this patch.
>>
>> I 100% agree with Julien on all points in this thread.
>>
>> Please can we keep the Linux S-o-b.
>>
>> Note that S-o-b is not a chain of *approval* (whose relevance to us is
>> debateable) but but a chain of *custody and transmission* for
>> copyright/licence/gdpr purposes.  That latter chain is hightly
>> relevant to us.
>>
>> All such S-o-b should be retained.
>>
>> Of course if you got the patch somewhere other than the Linux commit,
>> then the chain of custody doesn't pass through the Linux commit.  But
>> in that case I expect you to be able to say where you got it.
> 
> I've submitted v2 with S-o-b restored as far as necessary to meet this
> requirement. I did not restore all of them, because I continue to not
> see the value of retaining them. You saying "is highly relevant to us"
> is a statement, but not an explanation of why tags beyond those in the
> original submissions need retaining.
> 
> Without me seeing the need / value, I'm afraid I see only two ways
> forward: Either things are acceptable as they are now (and will be for
> future similar imports), or it needs to be someone else to put time
> into spotting and then pulling in such changes.

I am a bit confused how this would require more time. They are already 
in the commit message from Linus's git and you have a correct commit id. 
So this is merely a copy/paste.

I am not going to ack it but I am also not going to Nack it if another 
maintainer agrees with your approach.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 06.12.2021 15:28, Julien Grall wrote:
> On 06/12/2021 13:44, Jan Beulich wrote:
>> On 26.11.2021 13:52, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>>>> On 26.11.2021 11:04, Julien Grall wrote:
>>>>> For this case, you provided some sort of an explanation but so far, I am
>>>>> still waiting for a link to confirm that the signed-off-by match the one
>>>>> on the ML.
>>>>
>>>> I haven't been able to easily find a mail archive holding this patch.
>>>
>>> I 100% agree with Julien on all points in this thread.
>>>
>>> Please can we keep the Linux S-o-b.
>>>
>>> Note that S-o-b is not a chain of *approval* (whose relevance to us is
>>> debateable) but but a chain of *custody and transmission* for
>>> copyright/licence/gdpr purposes.  That latter chain is hightly
>>> relevant to us.
>>>
>>> All such S-o-b should be retained.
>>>
>>> Of course if you got the patch somewhere other than the Linux commit,
>>> then the chain of custody doesn't pass through the Linux commit.  But
>>> in that case I expect you to be able to say where you got it.
>>
>> I've submitted v2 with S-o-b restored as far as necessary to meet this
>> requirement. I did not restore all of them, because I continue to not
>> see the value of retaining them. You saying "is highly relevant to us"
>> is a statement, but not an explanation of why tags beyond those in the
>> original submissions need retaining.
>>
>> Without me seeing the need / value, I'm afraid I see only two ways
>> forward: Either things are acceptable as they are now (and will be for
>> future similar imports), or it needs to be someone else to put time
>> into spotting and then pulling in such changes.
> 
> I am a bit confused how this would require more time. They are already 
> in the commit message from Linus's git and you have a correct commit id. 
> So this is merely a copy/paste.

I didn't say "more time", did I? What I did (indirectly) say is that for
areas like this one it looks like I'm the only one to check at least
every once in a while. This has been working straightforwardly in the
past, but is now suddenly causing issues. And as indicated - if I would
understand the importance of tags which got mechanically added on the
way of flowing into Linux, I would likely be willing to give up my
position of viewing such extra tags as more getting in the way than
being helpful (much like I would always strip Cc: tags before committing,
as I firmly believe they have no place in the repo). But such an
explanation hasn't been given so far.

> I am not going to ack it but I am also not going to Nack it if another 
> maintainer agrees with your approach.

FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
I'm now in a position to put this in with Luca's R-b.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi,

On 06/12/2021 15:06, Jan Beulich wrote:
> On 06.12.2021 15:28, Julien Grall wrote:
>> On 06/12/2021 13:44, Jan Beulich wrote:
>>> On 26.11.2021 13:52, Ian Jackson wrote:
>>>> Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>>>>> On 26.11.2021 11:04, Julien Grall wrote:
>>>>>> For this case, you provided some sort of an explanation but so far, I am
>>>>>> still waiting for a link to confirm that the signed-off-by match the one
>>>>>> on the ML.
>>>>>
>>>>> I haven't been able to easily find a mail archive holding this patch.
>>>>
>>>> I 100% agree with Julien on all points in this thread.
>>>>
>>>> Please can we keep the Linux S-o-b.
>>>>
>>>> Note that S-o-b is not a chain of *approval* (whose relevance to us is
>>>> debateable) but but a chain of *custody and transmission* for
>>>> copyright/licence/gdpr purposes.  That latter chain is hightly
>>>> relevant to us.
>>>>
>>>> All such S-o-b should be retained.
>>>>
>>>> Of course if you got the patch somewhere other than the Linux commit,
>>>> then the chain of custody doesn't pass through the Linux commit.  But
>>>> in that case I expect you to be able to say where you got it.
>>>
>>> I've submitted v2 with S-o-b restored as far as necessary to meet this
>>> requirement. I did not restore all of them, because I continue to not
>>> see the value of retaining them. You saying "is highly relevant to us"
>>> is a statement, but not an explanation of why tags beyond those in the
>>> original submissions need retaining.
>>>
>>> Without me seeing the need / value, I'm afraid I see only two ways
>>> forward: Either things are acceptable as they are now (and will be for
>>> future similar imports), or it needs to be someone else to put time
>>> into spotting and then pulling in such changes.
>>
>> I am a bit confused how this would require more time. They are already
>> in the commit message from Linus's git and you have a correct commit id.
>> So this is merely a copy/paste.
> 
> I didn't say "more time", did I?
This seemed to be implied by asking someone else to do it.

> What I did (indirectly) say is that for
> areas like this one it looks like I'm the only one to check at least
> every once in a while. This has been working straightforwardly in the
> past, but is now suddenly causing issues. 

It is quite possible that this may have splipped in the previous review 
I have done. But now that I noticed it, I would like to confirm the 
signed-off-by was carried correctly.

> And as indicated - if I would
> understand the importance of tags which got mechanically added on the
> way of flowing into Linux, I would likely be willing to give up my
> position of viewing such extra tags as more getting in the way than
> being helpful (much like I would always strip Cc: tags before committing,
> as I firmly believe they have no place in the repo). But such an
> explanation hasn't been given so far.
The problem is how a reviewer can verify you did carry the tags properly 
when porting?

I agree that with the copy/paste, we may add mechanical tags. But it is 
reducing the effort for both the reviewer as they only need to check 
against the commit.

>> I am not going to ack it but I am also not going to Nack it if another
>> maintainer agrees with your approach.
> 
> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
> I'm now in a position to put this in with Luca's R-b.

 From the check-in policy section in MAINTAINERS:

4. There must be no "open" objections.

So I think this cannot be check-in given two maintainers disagree on the 
approach. That said, as I wrote earlier my condition for not Nacking is 
another maintainer agree with your approach.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 06.12.2021 17:06, Julien Grall wrote:
> On 06/12/2021 15:06, Jan Beulich wrote:
>> On 06.12.2021 15:28, Julien Grall wrote:
>>> I am not going to ack it but I am also not going to Nack it if another
>>> maintainer agrees with your approach.
>>
>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>> I'm now in a position to put this in with Luca's R-b.
> 
>  From the check-in policy section in MAINTAINERS:
> 
> 4. There must be no "open" objections.
> 
> So I think this cannot be check-in given two maintainers disagree on the 
> approach. That said, as I wrote earlier my condition for not Nacking is 
> another maintainer agree with your approach.

Hmm, I did address both your and Ian's concerns in v2, admittedly by only
going as far as minimally necessary. I therefore wouldn't call this an
"open objection".

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi Jan,

On 06/12/2021 16:12, Jan Beulich wrote:
> On 06.12.2021 17:06, Julien Grall wrote:
>> On 06/12/2021 15:06, Jan Beulich wrote:
>>> On 06.12.2021 15:28, Julien Grall wrote:
>>>> I am not going to ack it but I am also not going to Nack it if another
>>>> maintainer agrees with your approach.
>>>
>>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>>> I'm now in a position to put this in with Luca's R-b.
>>
>>   From the check-in policy section in MAINTAINERS:
>>
>> 4. There must be no "open" objections.
>>
>> So I think this cannot be check-in given two maintainers disagree on the
>> approach. That said, as I wrote earlier my condition for not Nacking is
>> another maintainer agree with your approach.
> 
> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
> going as far as minimally necessary. I therefore wouldn't call this an
> "open objection".

I believe my objection is still open. I still have have no way to verify 
what you did is correct.

For instance, the tags in patch #2 are:

Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
Reported-by: Yu Sun <yusun2@cisco.com>
Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
Acked-by: Daniel Walker <danielwa@cisco.com>
[Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]

The tags in the Linux commit are:

Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
Reported-by: Yu Sun <yusun2@cisco.com>
Acked-by: Daniel Walker <danielwa@cisco.com>
Cc: "Yixia Si (yisi)" <yisi@cisco.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

* The first two matches the original e-mails
* I couldn't find the 3rd on the ML.
* The Cc could be ignored
* The signed-off-by are I guess what you call "mechanical"

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 06.12.2021 17:21, Julien Grall wrote:
> On 06/12/2021 16:12, Jan Beulich wrote:
>> On 06.12.2021 17:06, Julien Grall wrote:
>>> On 06/12/2021 15:06, Jan Beulich wrote:
>>>> On 06.12.2021 15:28, Julien Grall wrote:
>>>>> I am not going to ack it but I am also not going to Nack it if another
>>>>> maintainer agrees with your approach.
>>>>
>>>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>>>> I'm now in a position to put this in with Luca's R-b.
>>>
>>>   From the check-in policy section in MAINTAINERS:
>>>
>>> 4. There must be no "open" objections.
>>>
>>> So I think this cannot be check-in given two maintainers disagree on the
>>> approach. That said, as I wrote earlier my condition for not Nacking is
>>> another maintainer agree with your approach.
>>
>> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
>> going as far as minimally necessary. I therefore wouldn't call this an
>> "open objection".
> 
> I believe my objection is still open. I still have have no way to verify 
> what you did is correct.
> 
> For instance, the tags in patch #2 are:
> 
> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
> Reported-by: Yu Sun <yusun2@cisco.com>
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> Acked-by: Daniel Walker <danielwa@cisco.com>
> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
> 
> The tags in the Linux commit are:
> 
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> Reported-by: Yu Sun <yusun2@cisco.com>
> Acked-by: Daniel Walker <danielwa@cisco.com>
> Cc: "Yixia Si (yisi)" <yisi@cisco.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> * The first two matches the original e-mails
> * I couldn't find the 3rd on the ML.
> * The Cc could be ignored
> * The signed-off-by are I guess what you call "mechanical"

Am I understanding right that now you're complaining about me
having retained one tag too many? So far all discussion was about
too few tags.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

On 06/12/2021 16:24, Jan Beulich wrote:
> On 06.12.2021 17:21, Julien Grall wrote:
>> On 06/12/2021 16:12, Jan Beulich wrote:
>>> On 06.12.2021 17:06, Julien Grall wrote:
>>>> On 06/12/2021 15:06, Jan Beulich wrote:
>>>>> On 06.12.2021 15:28, Julien Grall wrote:
>>>>>> I am not going to ack it but I am also not going to Nack it if another
>>>>>> maintainer agrees with your approach.
>>>>>
>>>>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>>>>> I'm now in a position to put this in with Luca's R-b.
>>>>
>>>>    From the check-in policy section in MAINTAINERS:
>>>>
>>>> 4. There must be no "open" objections.
>>>>
>>>> So I think this cannot be check-in given two maintainers disagree on the
>>>> approach. That said, as I wrote earlier my condition for not Nacking is
>>>> another maintainer agree with your approach.
>>>
>>> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
>>> going as far as minimally necessary. I therefore wouldn't call this an
>>> "open objection".
>>
>> I believe my objection is still open. I still have have no way to verify
>> what you did is correct.
>>
>> For instance, the tags in patch #2 are:
>>
>> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
>> Reported-by: Yu Sun <yusun2@cisco.com>
>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>> Acked-by: Daniel Walker <danielwa@cisco.com>
>> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
>>
>> The tags in the Linux commit are:
>>
>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>> Reported-by: Yu Sun <yusun2@cisco.com>
>> Acked-by: Daniel Walker <danielwa@cisco.com>
>> Cc: "Yixia Si (yisi)" <yisi@cisco.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> * The first two matches the original e-mails
>> * I couldn't find the 3rd on the ML.
>> * The Cc could be ignored
>> * The signed-off-by are I guess what you call "mechanical"
> 
> Am I understanding right that now you're complaining about me
> having retained one tag too many? So far all discussion was about
> too few tags.
I am complaining on the fact that this is really difficult to figure out 
how you decided which tags to keep.

I will repeat what I said before, it should have been so much easier if 
you just copied/pasted the one from the Linux commit.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Ian Jackson]

Julien Grall writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
> On 06/12/2021 16:12, Jan Beulich wrote:
> > Hmm, I did address both your and Ian's concerns in v2, admittedly by only
> > going as far as minimally necessary. I therefore wouldn't call this an
> > "open objection".
> 
> I believe my objection is still open. I still have have no way to verify 
> what you did is correct.

I can't believe this is still outstanding.  I think I understand
Julien's position, and I agree with what I have understood.

In particular, I think I understand why Julien feels it necessary to
make an issue of this.  The Signed-off-by lines are there to help
provide assurance that we aren't making legal mistakes.  They need to
be verifiable by a reviewer.  So that means that when a patch's own
declaration of its origin is "this patch came from Linux commit XYZ"
then all the S-o-b in that Linux git commit should be retained.

If the patch came from somewhere else, eg a mailing list post, I think
it would be OK to say something like "this patch came from lmkl, [url
to posting], and has since been committed to Linux as [commitid]~".
In that case the S-o-b should match the mailing list posting, but the
Xen patch being posted must then be identical to the mailing list
posting.

IOW it should be a deterministic process to start with the patch's
declaration of where it came from (or which sources it came from), and
verify that 1. the patch really did come from there and 2. all of the
approriate tags, especially S-o-b, are present.

By far the easiest way to achieve this is to take the patch from Linux
git using (eg) git-cherry-pick.  git will automatically DTRT. [1]

I don't have as strong an opinion about other tags, eg ones indicating
approval in Linux.  However, I think the overwhelming majority of
people would think it conventional to transfer all of the tags from
the original commit even if they are irrelevant in the new context.



I don't understand Jan's position.

Jan, why are you fighting so hard to delete these tags ?  What
practical harm does its presence do ?


[1] Jan, I know that you don't use git very much.  I think this is a
great shame.  I find it perplexing to see how anyone can work without
it.  The git command line UI is indeed terrible, but by now almost
everyone has either bitten the bullet of learning it, or adopted one
of the overlay UI packages that now exist.  (Personally I did learn
the cli but am starting to forget git cli nonsence since now I do
almost everything with magit inside emacs.)

I think the time has long passed when it is reasonable for a key Xen
developer to ask others to do additional work, or deal with anomalies,
in order to accomodate an unwillingness to use git.  Obviously we all
have our own workflows, but git has heavily influenced our shared
norms (and data formats).  If someone chooses not to use git, they
must at least be able to pretend.


Sorry,
Ian.

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 06.12.2021 17:45, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>> On 06/12/2021 16:12, Jan Beulich wrote:
>>> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
>>> going as far as minimally necessary. I therefore wouldn't call this an
>>> "open objection".
>>
>> I believe my objection is still open. I still have have no way to verify 
>> what you did is correct.
> 
> I can't believe this is still outstanding.

Same here, hardly surprising I suppose.

>  I think I understand
> Julien's position, and I agree with what I have understood.
> 
> In particular, I think I understand why Julien feels it necessary to
> make an issue of this.  The Signed-off-by lines are there to help
> provide assurance that we aren't making legal mistakes.  They need to
> be verifiable by a reviewer.  So that means that when a patch's own
> declaration of its origin is "this patch came from Linux commit XYZ"
> then all the S-o-b in that Linux git commit should be retained.
> 
> If the patch came from somewhere else, eg a mailing list post, I think
> it would be OK to say something like "this patch came from lmkl, [url
> to posting], and has since been committed to Linux as [commitid]~".
> In that case the S-o-b should match the mailing list posting, but the
> Xen patch being posted must then be identical to the mailing list
> posting.
> 
> IOW it should be a deterministic process to start with the patch's
> declaration of where it came from (or which sources it came from), and
> verify that 1. the patch really did come from there and 2. all of the
> approriate tags, especially S-o-b, are present.
> 
> By far the easiest way to achieve this is to take the patch from Linux
> git using (eg) git-cherry-pick.  git will automatically DTRT. [1]

It may be the easiest way, and I do understand Julien's replies in this
regard. This doesn't, however, mean that I agree the "easy" aspect
weighs higher than my view on stripping parts which aren't meaningful
in our tree (or, to be precise, which I think aren't meaningful; I will
leave it entirely open that I may be wrong with this).

As to git-cherry-pick: Just like I would have expected, even after
adding Linus'es tree as a remote this doesn't really work, due to the
different tree layouts:

$ git remote add --no-tags linus git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

$ git cherry-pick 83d3c4f22a
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 73569 and retry the command.
error: could not apply 83d3c4f22a36... lib/xz: Avoid overlapping memcpy() with invalid input with in-place decompression
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch smoke
Your branch is up to date with 'origin/smoke'.

You are currently cherry-picking commit 83d3c4f22a36.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

        deleted by us:   lib/decompress_unxz.c
        deleted by us:   lib/xz/xz_dec_lzma2.c

no changes added to commit (use "git add" and/or "git commit -a")

To adjust for our tree layout, manual intervention is necessary anyway
(unless there's a way to get "inexact rename detection" to actually
recognize the differences). What I take as a basis (git-format-patch
output or anything else) should be entirely up to me, I would say.

> I don't have as strong an opinion about other tags, eg ones indicating
> approval in Linux.  However, I think the overwhelming majority of
> people would think it conventional to transfer all of the tags from
> the original commit even if they are irrelevant in the new context.

Well, clearly I'm not part of this overwhelming majority then: I continue
to think that irrelevant things would better be omitted for clarity.

> I don't understand Jan's position.
> 
> Jan, why are you fighting so hard to delete these tags ?

Counter question: Why are you and Julien fighting so hard for the
retaining of what I consider inapplicable information? But to answer
your question: Just like I consider missing information a problem, I do
also consider meaningless data a problem. Plus of course there's now
the psychological effect resulting from already having invested far
more time here than I think any one of us should have invested: I now
absolutely want to understand whether I have been doing things wrong for
years. As said, me doing what I have done here hasn't been a problem
before.

It's still not clear to me whether I'm doing anything wrong in the first
place - my question as to why these tags are relevant in our trees has
remained unanswered. I've actually taken the time to dig out "Developer's
Certificate of Origin 1.1" - from all I can tell my submission matches (b)
(and that's imo true even for v1, i.e. before re-adding some of the tags
to match the submissions according to mailing list archives, as far as
entries are available).

In this context I'd also like to point out that unlike Linux we don't
normally make use of (c).

> What practical harm does its presence do ?

I did answer this one earlier on as well as above: I consider their
presence inapplicable. There's no severe "harm", but there's also no
reason I know to keep them. Unless I'm told of a reason, I view it as
the submitter's choice to keep or strip such. In fact, if I saw an
import which had such seemingly stray tags, I probably wouldn't insist
on stripping them, but I might question their presence / applicability.

> [1] Jan, I know that you don't use git very much.  I think this is a
> great shame.  I find it perplexing to see how anyone can work without
> it.  The git command line UI is indeed terrible, but by now almost
> everyone has either bitten the bullet of learning it, or adopted one
> of the overlay UI packages that now exist.  (Personally I did learn
> the cli but am starting to forget git cli nonsence since now I do
> almost everything with magit inside emacs.)
> 
> I think the time has long passed when it is reasonable for a key Xen
> developer to ask others to do additional work, or deal with anomalies,
> in order to accomodate an unwillingness to use git.  Obviously we all
> have our own workflows, but git has heavily influenced our shared
> norms (and data formats).  If someone chooses not to use git, they
> must at least be able to pretend.

I appreciate (yet don't share) your view, but primarily I'm afraid I
don't view this rant as relevant in this context. Even if I had found
a way to do the import entirely via git commands, I likely would have
edited the description to strip tags alongside adding my S-o-b (the
latter I understand I could have git do for me). Since git-cherry-pick
didn't work for me here, I also can't tell whether the usual
"(cherry-picked from ...)" that git can be told to add would actually
have identified the different tree, or whether I would have had to add
that to make the referenced hash meaningful.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 06.12.2021 17:21, Julien Grall wrote:
> Hi Jan,
> 
> On 06/12/2021 16:12, Jan Beulich wrote:
>> On 06.12.2021 17:06, Julien Grall wrote:
>>> On 06/12/2021 15:06, Jan Beulich wrote:
>>>> On 06.12.2021 15:28, Julien Grall wrote:
>>>>> I am not going to ack it but I am also not going to Nack it if another
>>>>> maintainer agrees with your approach.
>>>>
>>>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>>>> I'm now in a position to put this in with Luca's R-b.
>>>
>>>   From the check-in policy section in MAINTAINERS:
>>>
>>> 4. There must be no "open" objections.
>>>
>>> So I think this cannot be check-in given two maintainers disagree on the
>>> approach. That said, as I wrote earlier my condition for not Nacking is
>>> another maintainer agree with your approach.
>>
>> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
>> going as far as minimally necessary. I therefore wouldn't call this an
>> "open objection".
> 
> I believe my objection is still open.

I've taken note of this. I'm afraid with the long winded discussion no
other maintainer will provide an ack. Which therefore makes what you said
above effectively a nak anyway. Unless things move in unexpected ways, I
will have to consider this series rejected then.

> I still have have no way to verify 
> what you did is correct.
> 
> For instance, the tags in patch #2 are:
> 
> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
> Reported-by: Yu Sun <yusun2@cisco.com>
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> Acked-by: Daniel Walker <danielwa@cisco.com>
> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
> 
> The tags in the Linux commit are:
> 
> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
> Reported-by: Yu Sun <yusun2@cisco.com>
> Acked-by: Daniel Walker <danielwa@cisco.com>
> Cc: "Yixia Si (yisi)" <yisi@cisco.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> * The first two matches the original e-mails
> * I couldn't find the 3rd on the ML.

See e.g.

https://yhbt.net/lore/all/20191108202754.GG18744@zorba/t/

(Andrew Morton's reply at the bottom) for where it originates.

> * The Cc could be ignored
> * The signed-off-by are I guess what you call "mechanical"

I would generally retain Reviewed-by when our code is still quite
similar to Linux'es. Acked-by are on the edge of being useful, but as
you can see I did err on the side of keeping it. As said in a number
of places elsewhere, for what I call mechanically added tags I am yet
to be told of their value (or even need) in our tree. Not the least
- as also said in reply to Ian - because we don't usually follow
Linux'es model of flowing patches through several trees, where each
tree owner would apply their S-o-b as per (c) of "Developer's
Certificate of Origin 1.1".

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Julien Grall]

Hi,

On 07/12/2021 09:11, Jan Beulich wrote:
> On 06.12.2021 17:21, Julien Grall wrote:
>> Hi Jan,
>>
>> On 06/12/2021 16:12, Jan Beulich wrote:
>>> On 06.12.2021 17:06, Julien Grall wrote:
>>>> On 06/12/2021 15:06, Jan Beulich wrote:
>>>>> On 06.12.2021 15:28, Julien Grall wrote:
>>>>>> I am not going to ack it but I am also not going to Nack it if another
>>>>>> maintainer agrees with your approach.
>>>>>
>>>>> FTAOD I'll be giving it a week or so, but unless I get an outright NAK,
>>>>> I'm now in a position to put this in with Luca's R-b.
>>>>
>>>>    From the check-in policy section in MAINTAINERS:
>>>>
>>>> 4. There must be no "open" objections.
>>>>
>>>> So I think this cannot be check-in given two maintainers disagree on the
>>>> approach. That said, as I wrote earlier my condition for not Nacking is
>>>> another maintainer agree with your approach.
>>>
>>> Hmm, I did address both your and Ian's concerns in v2, admittedly by only
>>> going as far as minimally necessary. I therefore wouldn't call this an
>>> "open objection".
>>
>> I believe my objection is still open.
> 
> I've taken note of this. I'm afraid with the long winded discussion no
> other maintainer will provide an ack. Which therefore makes what you said
> above effectively a nak anyway. Unless things move in unexpected ways, I
> will have to consider this series rejected then.

The code is itself is fine. I would be fine to ack them so long I can 
verify the tags you carried.

As I wrote multiple time the easiest way here it to copy/paste them. 
They may be meaningless to you but it is going to save a lot of time for 
me to verify you carried the tags correctly.

But see more below.

>> I still have have no way to verify
>> what you did is correct.
>>
>> For instance, the tags in patch #2 are:
>>
>> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
>> Reported-by: Yu Sun <yusun2@cisco.com>
>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>> Acked-by: Daniel Walker <danielwa@cisco.com>
>> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
>>
>> The tags in the Linux commit are:
>>
>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>> Reported-by: Yu Sun <yusun2@cisco.com>
>> Acked-by: Daniel Walker <danielwa@cisco.com>
>> Cc: "Yixia Si (yisi)" <yisi@cisco.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> * The first two matches the original e-mails
>> * I couldn't find the 3rd on the ML.
> 
> See e.g.
> 
> https://yhbt.net/lore/all/20191108202754.GG18744@zorba/t/
> 
> (Andrew Morton's reply at the bottom) for where it originates.

Ok... So this is taken from a different aggregator. I will have to brush 
by search engine skill then.

> 
>> * The Cc could be ignored
>> * The signed-off-by are I guess what you call "mechanical"
> 
> I would generally retain Reviewed-by when our code is still quite
> similar to Linux'es. Acked-by are on the edge of being useful, but as
> you can see I did err on the side of keeping it. As said in a number
> of places elsewhere, for what I call mechanically added tags I am yet
> to be told of their value (or even need) in our tree.

I think the question is how difficult to do you want to make to the 
other reviewers? I appreciate other (including myself) may have ignored 
the tags in the past. But now that I know you do it as a manual process, 
it makes me a lot more nervous to simply ack such patch without any check.

You seem to be unwilling to simply copy/paste them. So for this series, 
would you be happy if someone else do it for you?

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 07.12.2021 10:59, Julien Grall wrote:
> On 07/12/2021 09:11, Jan Beulich wrote:
>> On 06.12.2021 17:21, Julien Grall wrote:
>>> I still have have no way to verify
>>> what you did is correct.
>>>
>>> For instance, the tags in patch #2 are:
>>>
>>> Link: http://lkml.kernel.org/r/20191104185107.3b6330df@tukaani.org
>>> Reported-by: Yu Sun <yusun2@cisco.com>
>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>> Acked-by: Daniel Walker <danielwa@cisco.com>
>>> [Linux commit: 8e20ba2e53fc6198cbfbcc700e9f884157052a8d]
>>>
>>> The tags in the Linux commit are:
>>>
>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>> Reported-by: Yu Sun <yusun2@cisco.com>
>>> Acked-by: Daniel Walker <danielwa@cisco.com>
>>> Cc: "Yixia Si (yisi)" <yisi@cisco.com>
>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>>
>>> * The first two matches the original e-mails
>>> * I couldn't find the 3rd on the ML.
>>
>> See e.g.
>>
>> https://yhbt.net/lore/all/20191108202754.GG18744@zorba/t/
>>
>> (Andrew Morton's reply at the bottom) for where it originates.
> 
> Ok... So this is taken from a different aggregator. I will have to brush 
> by search engine skill then.

To be fair, I went hunting for it only when writing the earlier reply.

>>> * The Cc could be ignored
>>> * The signed-off-by are I guess what you call "mechanical"
>>
>> I would generally retain Reviewed-by when our code is still quite
>> similar to Linux'es. Acked-by are on the edge of being useful, but as
>> you can see I did err on the side of keeping it. As said in a number
>> of places elsewhere, for what I call mechanically added tags I am yet
>> to be told of their value (or even need) in our tree.
> 
> I think the question is how difficult to do you want to make to the 
> other reviewers? I appreciate other (including myself) may have ignored 
> the tags in the past. But now that I know you do it as a manual process, 
> it makes me a lot more nervous to simply ack such patch without any check.
> 
> You seem to be unwilling to simply copy/paste them.

I'm unwilling only as long as I don't understand the need for them. As
indicated, while I appreciate your "make verification easier for
reviewers", I assign that at least no higher priority than my desire
to leave out inapplicable data.

> So for this series, would you be happy if someone else do it for you?

I'd be happy for anyone else to start over. I would even ack such a
submission myself. But as long as I'm recorded with S-o-b, I'm afraid
I'm not going to accept re-addition of the tags for no good (as per my
personal view) reason. Otherwise, based on experience, the example of
this series could, in the future, be used to tell me that on an earlier
occasion I (seemingly) did things differently.

As said earlier, if submissions in this form are going to be nak-ed
going forward, and if good reasons (see above) will not be provided
(and hence leeway will not be granted to the submitter) to support this,
then someone else will need to start looking after imports which may be
relevant to us.

Jan

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
> I'm unwilling only as long as I don't understand the need for them. As
> indicated, while I appreciate your "make verification easier for
> reviewers", I assign that at least no higher priority than my desire
> to leave out inapplicable data.

Are we still talking about Signed-off-by ?

I explained the purpose of Signed-off-by.  It reflects the chain of
custody.  It is true that s-o-b is often added by people with minimal
contribution to the content; I don't think that is relevant.

If the Xen patch was derived from Linux commit XYZ (whether
automatically or manually) then even those minimal S-o-b are
applicable.

> I'd be happy for anyone else to start over. I would even ack such a
> submission myself. But as long as I'm recorded with S-o-b, I'm afraid
> I'm not going to accept re-addition of the tags for no good (as per my
> personal view) reason. Otherwise, based on experience, the example of
> this series could, in the future, be used to tell me that on an earlier
> occasion I (seemingly) did things differently.

S-o-b does not indicate complete approval of the content.  It attests
precisely to the statements in the DCO.  There is nothing irregular
about putting your S-o-b on something you don't entirely agree with.


Stepping back:

In a collaborative project, we must all respect our peers and the
community's decision.  That can mean actively progressing patches that
we personally disagree with, but which the community has decided ought
to be applied. [1]

I appreciate that can be less fun.  But it comes with the territory of
being a leading member of the community.


> As said earlier, if submissions in this form are going to be nak-ed
> going forward, and if good reasons (see above) will not be provided
> (and hence leeway will not be granted to the submitter) to support this,
> then someone else will need to start looking after imports which may be
> relevant to us.

The problem is simply one of verification.  So far there does not seem
to be a positive ack[1] for this patch.  Neither I nor Julien are
nacking it.

AIUI Julien does not want to ack it without being able to check that
all the appropriate s-o-b (and perhaps other tags) are present.  I
think that as the submitter it is really best if you make that easy.


We think the obvious way to make that easy for a reviewer is to just
copy the tags.  But an alternative would be to include, in the commit
message, full details of where the patch came from in (including urls
to mailing list articles) in such a way that a reviewer can see easily
that all the necessary s-o-b are present.


[1] Of course very rarely there might be matters of conscience, where
we have protested but our protest has been overruled by our peers.
But that does not seem to be the case here since you are not giving a
nak.

Ian.

[1] Julien did give one earlier but then wrote "actually" and started
this subthread, so I think Julien has withdrawn his ack.

Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

[Jan Beulich]

On 07.12.2021 13:00, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 1/7] xz: add fall-through comments to a switch statement"):
>> I'm unwilling only as long as I don't understand the need for them. As
>> indicated, while I appreciate your "make verification easier for
>> reviewers", I assign that at least no higher priority than my desire
>> to leave out inapplicable data.
> 
> Are we still talking about Signed-off-by ?
> 
> I explained the purpose of Signed-off-by.  It reflects the chain of
> custody.

And I accepted that (without claiming to truly understand things) as
far as the desire to prove all tags goes. Hence the v2 submission. To
me, as can be seen there, that chain of custody includes the original
patch submission, but not the flow through Linux subsystem trees.

>  It is true that s-o-b is often added by people with minimal
> contribution to the content; I don't think that is relevant.
> 
> If the Xen patch was derived from Linux commit XYZ (whether
> automatically or manually) then even those minimal S-o-b are
> applicable.

I'd like to adjust that to "If the Xen patch was claimed to have
been derived from Linux commit XYZ ..." I don't think I've made such
a claim anywhere.

>> I'd be happy for anyone else to start over. I would even ack such a
>> submission myself. But as long as I'm recorded with S-o-b, I'm afraid
>> I'm not going to accept re-addition of the tags for no good (as per my
>> personal view) reason. Otherwise, based on experience, the example of
>> this series could, in the future, be used to tell me that on an earlier
>> occasion I (seemingly) did things differently.
> 
> S-o-b does not indicate complete approval of the content.  It attests
> precisely to the statements in the DCO.  There is nothing irregular
> about putting your S-o-b on something you don't entirely agree with.

Isn't it up to me where I do put my S-o-b under, and where I'd rather
not?

> Stepping back:
> 
> In a collaborative project, we must all respect our peers and the
> community's decision.  That can mean actively progressing patches that
> we personally disagree with, but which the community has decided ought
> to be applied. [1]

The latter part aside (as I don't think I'm standing in the way of
getting these changes in), I don't see any "community decision" here.
What to do (or not) in cases like pulling in changes from elsewhere
is simply not documented anywhere. Hence all I could have gone from
are past examples. (I don't dare to guess what the correct thing to
do was if a change was to be taken from a project not using the S-o-b
model.)

>> As said earlier, if submissions in this form are going to be nak-ed
>> going forward, and if good reasons (see above) will not be provided
>> (and hence leeway will not be granted to the submitter) to support this,
>> then someone else will need to start looking after imports which may be
>> relevant to us.
> 
> The problem is simply one of verification.  So far there does not seem
> to be a positive ack[1] for this patch.  Neither I nor Julien are
> nacking it.

But, as Julien calls it, "open objections" are effectively preventing
the thing from going in. Otherwise Luca's R-b would be enough for the
whole lot to go in.

I did submit v2 with his ack on patch 1, in the belief that a) he didn't
actively withdraw it and b) I did address his concerns there. Earlier
today, seeing the ongoing discussion, I did drop it here (and I'll try
to remember to also reply to this effect to the patch itself).

> AIUI Julien does not want to ack it without being able to check that
> all the appropriate s-o-b (and perhaps other tags) are present.  I
> think that as the submitter it is really best if you make that easy.
> 
> 
> We think the obvious way to make that easy for a reviewer is to just
> copy the tags.  But an alternative would be to include, in the commit
> message, full details of where the patch came from in (including urls
> to mailing list articles) in such a way that a reviewer can see easily
> that all the necessary s-o-b are present.

This alternative is what v2 does: Already in v1 there were links
present to the original submissions, where available. I didn't think
there was any extra wording needed next to the links. v2 merely syncs
the tags with the patch submissions.

> [1] Of course very rarely there might be matters of conscience, where
> we have protested but our protest has been overruled by our peers.
> But that does not seem to be the case here since you are not giving a
> nak.
> 
> Ian.
> 
> [1] Julien did give one earlier but then wrote "actually" and started
> this subthread, so I think Julien has withdrawn his ack.

Well, okay, I didn't read it that way, especially since I did address
his concern there in v2. But as said, I've meanwhile dropped his ack
from there.

Jan