Re: [PATCH v2 3/5] zstd: move ZSTD_MAX_CLEVEL to zstd_lib.h

On (26/07/29 12:32), haoqin huang wrote:
> > > > > -#define ZSTD_MAX_CLEVEL     22
> > > > > -
> > > > >  __attribute__((__unused__))
> > > >
> > > > Sashiko made a good point.  Can we use zstd_max_clevel() instead?
> > >
> > > Good point, I'll drop this patch and use zstd_max_clevel() instead
> > > in v3.
> > >
> > > Since it's a runtime function and can't be used for static struct
> > > initialization, I plan to set backend_zstd's level_max to -1 as a
> > > sentinel value, and query the actual maximum in
> > > zcomp_validate_params():
> > >
> > >     s32 max = backend->level_max;
> > >     if (max < 0)
> > >         max = zstd_max_clevel();
> > >
> > > Do you think this approach is feasible?
> >
> > Hmm, no, that doesn't look good. zcomp should not include
> > zstd.h or any other libs directly.  Should params validation
> > be a per-backend callback then?
> 
> Good point, zcomp.c shouldn't include library headers.  A per-backend
> callback would be cleaner.
> 
> I'll add an optional validate_params to zcomp_ops: the zstd backend
> implements it using zstd_max_clevel() internally, while lzo/deflate
> and others just rely on the static caps check (no callback needed).
> zcomp.c stays free of any library headers.

I'm actually leaning towards validation in .setup_params() now.
It's not immediate but, first, it doesn't matter that much, we still
don't create zram with invalid params configuration and, second, it
sort of makes sense to do validation in .setup_params().  We already
started doing that for deflate winbits (a patch from earlier today).

Can you please add your validation to per-backend .setup_params()?