On Thu, Oct 17, 2019 at 11:42:53AM +0200, Johannes Thumshirn wrote:
> On 14/10/2019 14:22, David Sterba wrote:
> > +static int compression_decompress_bio(int type, struct list_head *ws,
> > + struct compressed_bio *cb)
> > +{
> > + switch (type) {
> > + case BTRFS_COMPRESS_ZLIB: return zlib_decompress_bio(ws, cb);
> > + case BTRFS_COMPRESS_LZO: return lzo_decompress_bio(ws, cb);
> > + case BTRFS_COMPRESS_ZSTD: return zstd_decompress_bio(ws, cb);
> > + case BTRFS_COMPRESS_NONE:
> > + default:
> > + /*
> > + * This can't happen, the type is validated several times
> > + * before we get here.
> > + */
> > + BUG();
> > + }
> > +}
> > +
> > +static int compression_decompress(int type, struct list_head *ws,
> > + unsigned char *data_in, struct page *dest_page,
> > + unsigned long start_byte, size_t srclen, size_t destlen)
> > +{
> > + switch (type) {
> > + case BTRFS_COMPRESS_ZLIB: return zlib_decompress(ws, data_in, dest_page,
> > + start_byte, srclen, destlen);
> > + case BTRFS_COMPRESS_LZO: return lzo_decompress(ws, data_in, dest_page,
> > + start_byte, srclen, destlen);
> > + case BTRFS_COMPRESS_ZSTD: return zstd_decompress(ws, data_in, dest_page,
> > + start_byte, srclen, destlen);
> > + case BTRFS_COMPRESS_NONE:
> > + default:
> > + /*
> > + * This can't happen, the type is validated several times
> > + * before we get here.
> > + */
> > + BUG();
> > + }
> > +}
>
> Hmm should we really BUG() here? Maybe throw in an ASSERT(), so we can
> catch it in debug builds but not halt the machine (even if it
> theoretically can't happen).
Debug builds would catch it for sure, regardless of bug or assert, but
assert would not be on builds without CONFIG_BTRFS_ASSERT and continuing
here is not correct. If the code reatches the switch with a wrong value,
then the wrong value has been used to dereference an array in the
caller, so the state of the system is not defined. I don't see a better
option than BUG, thouth we don't want to add new ones. At least with the
comment it should be clear that it's not somebody's laziness to do
proper error handling, here it's not possible to do it at all.
