> On Oct 11, 2018, at 10:55 AM, Daniel Kiper <dkiper@xxxxxxxxxxxx> wrote:
>
> On Tue, Oct 09, 2018 at 04:21:37PM -0700, Nick Terrell wrote:
>> Adds zstd support to the btrfs module.
>>
>> Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
>> compression. A test case was also added to the test suite that fails before
>> the patch, and passes after.
>>
>> Signed-off-by: Nick Terrell <terrelln@xxxxxx>
>> ---
>> v1 -> v2:
>> - Fix comments from Daniel Kiper.
>>
>> v2 -> v3:
>> - Use grub_error() to set grub_errno in grub_btrfs_zstd_decompress().
>> - Fix style and formatting comments.
>>
>> Makefile.util.def | 10 +++-
>> grub-core/Makefile.core.def | 10 +++-
>> grub-core/fs/btrfs.c | 112 ++++++++++++++++++++++++++++++++++-
>> tests/btrfs_test.in | 1 +
>> tests/util/grub-fs-tester.in | 2 +-
>> 5 files changed, 131 insertions(+), 4 deletions(-)
>>
>> diff --git a/Makefile.util.def b/Makefile.util.def
>> index f9caccb97..27c5e9903 100644
>> --- a/Makefile.util.def
>> +++ b/Makefile.util.def
>> @@ -54,7 +54,7 @@ library = {
>> library = {
>> name = libgrubmods.a;
>> cflags = '-fno-builtin -Wno-undef';
>> - cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H';
>> + cppflags = '-I$(srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -I$(srcdir)/grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H';
>>
>> common_nodist = grub_script.tab.c;
>> common_nodist = grub_script.yy.c;
>> @@ -164,6 +164,14 @@ library = {
>> common = grub-core/lib/xzembed/xz_dec_bcj.c;
>> common = grub-core/lib/xzembed/xz_dec_lzma2.c;
>> common = grub-core/lib/xzembed/xz_dec_stream.c;
>> + common = grub-core/lib/zstd/debug.c;
>> + common = grub-core/lib/zstd/entropy_common.c;
>> + common = grub-core/lib/zstd/error_private.c;
>> + common = grub-core/lib/zstd/fse_decompress.c;
>> + common = grub-core/lib/zstd/huf_decompress.c;
>> + common = grub-core/lib/zstd/xxhash.c;
>> + common = grub-core/lib/zstd/zstd_common.c;
>> + common = grub-core/lib/zstd/zstd_decompress.c;
>> };
>>
>> program = {
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 2c1d62cee..f818f58bc 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1265,8 +1265,16 @@ module = {
>> name = btrfs;
>> common = fs/btrfs.c;
>> common = lib/crc.c;
>> + common = lib/zstd/debug.c;
>> + common = lib/zstd/entropy_common.c;
>> + common = lib/zstd/error_private.c;
>> + common = lib/zstd/fse_decompress.c;
>> + common = lib/zstd/huf_decompress.c;
>> + common = lib/zstd/xxhash.c;
>> + common = lib/zstd/zstd_common.c;
>> + common = lib/zstd/zstd_decompress.c;
>> cflags = '$(CFLAGS_POSIX) -Wno-undef';
>> - cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo -DMINILZO_HAVE_CONFIG_H';
>> + cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo -I$(srcdir)/lib/zstd -DMINILZO_HAVE_CONFIG_H';
>> };
>
> Please do not embed zstd lib into btrfs module here.
> I would like to see zstd lib as separate module if possible.
Sure, I'm not exactly sure how the build system works. I haven't been able to
find any documentation, if there is some can you point me to it? I think the zstd
module should look like:
module = {
name = zstd;
common = lib/zstd/debug.c;
common = lib/zstd/entropy_common.c;
common = lib/zstd/error_private.c;
common = lib/zstd/fse_decompress.c;
common = lib/zstd/huf_decompress.c;
common = lib/zstd/xxhash.c;
common = lib/zstd/zstd_common.c;
common = lib/zstd/zstd_decompress.c;
cflags = '$(CFLAGS_POSIX) -Wno-undef';
cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/zstd';
};
Then how do I add a dependency on it in btrfs?
>> +static grub_ssize_t
>> +grub_btrfs_zstd_decompress (char *ibuf, grub_size_t isize, grub_off_t off,
>> + char *obuf, grub_size_t osize)
>> +{
>> + void *allocated = NULL;
>> + char *otmpbuf = obuf;
>> + grub_size_t otmpsize = osize;
>> + ZSTD_DCtx *dctx = NULL;
>> + grub_size_t zstd_ret;
>> + grub_ssize_t ret = -1;
>> +
>> + /*
>> + * Zstd will fail if it can't fit the entire output in the destination
>> + * buffer, so if osize isn't large enough, allocate a temporary buffer.
>> + */
>> + if (otmpsize < ZSTD_BTRFS_MAX_INPUT) {
>> + allocated = grub_malloc (ZSTD_BTRFS_MAX_INPUT);
>> + if (!allocated) {
>
> Hmmm... Should not we say something here? Like grub_error() call below?
> It seems to me that failing silently is bad idea here.
grub_malloc() already calls grub_error with the message "Out of memory".
Thanks,
Nick
