On Wed, Sep 4, 2024 at 7:56 PM Gao Xiang <hsiang...@linux.alibaba.com> wrote:
>
> On 2024/8/31 10:58, Gao Xiang wrote:
> > Hi Sandeep,
> >
> > On 2024/8/31 05:46, Sandeep Dhavale wrote:
> >> Hi Liujinbao,
> >> On Thu, Aug 29, 2024 at 5:24 AM liujinbao1 <jinbaoliu...@gmail.com> wrote:
> >>>
> >>> From: liujinbao1 <liujinb...@xiaomi.com>
> >>>
> >>> When i=0 and err is not equal to 0,
> >>> the while(-1) loop will enter into an
> >>> infinite loop. This patch avoids this issue
> >>>
> >>> Signed-off-by: liujinbao1 <liujinb...@xiaomi.com>
> >>> ---
> >>> fs/erofs/decompressor.c | 10 +++++-----
> >>> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> >>> index c2253b6a5416..672f097966fa 100644
> >>> --- a/fs/erofs/decompressor.c
> >>> +++ b/fs/erofs/decompressor.c
> >>> @@ -534,18 +534,18 @@ int z_erofs_parse_cfgs(struct super_block *sb,
> >>> struct erofs_super_block *dsb)
> >>>
> >>> int __init z_erofs_init_decompressor(void)
> >>> {
> >>> - int i, err;
> >>> + int i, err = 0;
> >>>
> >>> for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
> >>> err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
> >>> - if (err) {
> >>> - while (--i)
> >>> + if (err && i) {
> >>> + while (i--)
> >> Actually there is a subtle bug in this fix. We will never enter the if
> >> block here when i=0 and err is set which we were trying to fix.
> >> This will cause z_erofs_decomp[0]->init() error to get masked and we
> >> will continue the outer for loop (i.e. when i=0 and err is set).
> >
>
Hi Gao,
> Ping? could anyone submit a proper fix for this?
>
> Yes, that needs to be resolved, and I will replace the patch to the new
> version.
I think I misunderstood this as you are amending it.
> Or just
>
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index c2253b6a5416..dfb77f4e68b4 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -539,8 +539,8 @@ int __init z_erofs_init_decompressor(void)
> for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
> err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
> if (err) {
> - while (--i)
> - if (z_erofs_decomp[i])
> + while (i)
> + if (z_erofs_decomp[--i])
> z_erofs_decomp[i]->exit();
Even though logically same, decrementing `i` in the middle of 3 lines
referencing it does not feel very readable to me.
A below version is more traditional as changing the cond in the while() check.
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index c2253b6a5416..eb318c7ddd80 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -539,7 +539,7 @@ int __init z_erofs_init_decompressor(void)
for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
if (err) {
- while (--i)
+ while (i--)
if (z_erofs_decomp[i])
z_erofs_decomp[i]->exit();
return err;
I will send this in a formal patch in few mins as v3, if you feel ok,
please add it. If you want to stick to your version, no problem, I
understand that it is subjective.
Thanks,
Sandeep.
> return err;
> }
>
> to avoid underflowed `i` (although it should have no real impact.)
>
> Thanks,
> Gao Xiang
