Re: [PATCH v5 3/4] zram: validate parameters in each backend's setup_params

On (26/08/04 15:02), haoqin huang wrote:
> On Tue, Aug 4, 2026 at 1:37 PM Sergey Senozhatsky
> <[email protected]> wrote:
> >
> > On (26/08/03 22:12), Haoqin Huang wrote:
> > >  static int deflate_setup_params(struct zcomp_params *params)
> > >  {
> > > +     if (params->dict_sz) {
> > > +             pr_err("deflate: dictionary is not supported\n");
> > > +             return -EOPNOTSUPP;
> > > +     }
> > > +
> > >       if (params->level == ZCOMP_PARAM_NOT_SET)
> > >               params->level = Z_DEFAULT_COMPRESSION;
> >
> > If we want to be pedantic, then {} should also be added to the "if"
> > in this case.  And in other similar cases.
> >
> 
> Okay, thanks for the reminder. I will add {} to the if-branch for consistency.

Thanks.

> > > +     else if (params->level < Z_DEFAULT_COMPRESSION ||
> > > +              params->level > Z_BEST_COMPRESSION) {
> > > +             pr_err("deflate: invalid compression level %d\n", params->level);
> > > +             return -EINVAL;
> > > +     }
> > > +
> >
> > [..]
> > > +     else if (params->level < 1 || params->level > LZ4HC_MAX_CLEVEL) {
> > > +             pr_err("lz4hc: invalid compression level %d\n", params->level);
> > > +             return -EINVAL;
> > > +     }
> >
> > Let's add a small comment justifying/explaining that "1" constant?
> 
> For the lz4hc comment, how about:
>         /*
>          * LZ4HC_compress_generic() clamps levels below 1 to
>          * LZ4HC_DEFAULT_CLEVEL, so < 1 is the real lower bound.
>          */
> Does that look reasonable?

I suppose we want to document the fact that LZ4HC_MIN_CLEVEL (which
would be naturally expected here) is set to 3 and cuts off levels 1
and 2 which are perfectly valid.  Maybe just say something like that,
for simplicity.