On 2025/12/2 16:43, Gao Xiang wrote:
Hi Dan,
On 2025/12/2 16:32, Dan Carpenter wrote:
Hello Gao Xiang,
Commit 30e13e41a0eb ("erofs: enable error reporting for
z_erofs_fixup_insize()") from Nov 27, 2025 (linux-next), leads to the
following Smatch static checker warning:
fs/erofs/decompressor.c:217 z_erofs_lz4_decompress_mem() warn: 'reason' isn't
an ERR_PTR
fs/erofs/decompressor_lzma.c:193 z_erofs_lzma_decompress() warn: 'reason' is an
error pointer or valid
fs/erofs/decompressor_zstd.c:180 z_erofs_zstd_decompress() warn: 'reason' is an
error pointer or valid
BTW, I don't think any issue out of `fs/erofs/decompressor_lzma.c`
and `fs/erofs/decompressor_zstd.c`.
"reason != NULL" means failure, so it will break and propagate to
the caller.
fs/erofs/decompressor.c
198 static int z_erofs_lz4_decompress_mem(struct z_erofs_decompress_req
*rq, u8 *dst)
199 {
200 bool support_0padding = false, may_inplace = false;
201 unsigned int inputmargin;
202 u8 *out, *headpage, *src;
203 const char *reason;
204 int ret, maptype;
205
206 DBG_BUGON(*rq->in == NULL);
207 headpage = kmap_local_page(*rq->in);
208
209 /* LZ4 decompression inplace is only safe if zero_padding is
enabled */
210 if (erofs_sb_has_zero_padding(EROFS_SB(rq->sb))) {
211 support_0padding = true;
212 reason = z_erofs_fixup_insize(rq, headpage +
rq->pageofs_in,
213 min_t(unsigned int, rq->inputsize,
214 rq->sb->s_blocksize -
rq->pageofs_in));
215 if (reason) {
216 kunmap_local(headpage);
--> 217 return IS_ERR(reason) ? PTR_ERR(reason) :
-EFSCORRUPTED;
218 }
The z_erofs_fixup_insize() function used to return error pointers, but
now it returns an error string or NULL. So probably we could just
change this to:
return -EFSCORRUPTED;
I think `return -EFSCORRUPTED;` is valid for now, but later
z_erofs_lz4_decompress_mem() will be changed into returning
`const char *` as well but it's late for this cycle.
Are we planning to make it return error pointers again?
z_erofs_lz4_decompress_mem() will be `const char *` later too
along with other cleanups, but it's somewhat late for this cycle.
NULL means success in this case, right? It's sort of weird how NULL means
success and a valid pointer means failure.
Because -EFSCORRUPTED isn't enough to represent the detailed
decompression errors (because each decompressor has its own
error messages and it's helpful for users to know how the
compressed data is corrupted).
NULL means success, and both valid pointers or error pointers
!= NULL means failure, so personally I don't think it's weird.
A valid pointer indicates an detailed error string to be
printed so that users know the detailed reasons and the error
number is -EFSCORRUPTED.
Thanks,
Gao Xiang
