On 3/7/18 1:34 AM, Qu Wenruo wrote: > > > On 2018年03月03日 02:47, je...@suse.com wrote: >> diff --git a/configure.ac b/configure.ac >> index 56d17c3a..6aec672a 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -197,6 +197,12 @@ PKG_STATIC(UUID_LIBS_STATIC, [uuid]) >> PKG_CHECK_MODULES(ZLIB, [zlib]) >> PKG_STATIC(ZLIB_LIBS_STATIC, [zlib]) >> >> +PKG_CHECK_MODULES(JSON, [json-c], [ > > Json-c is quite common and also used by cryptsetup, so pretty good > library choice.
Yep, so that puts it in the base system packages of most distros.
>> diff --git a/qgroup.c b/qgroup.c
>> index 2d0a6947..f632a45c 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>> return ret;
>> }
>>
>> +#ifdef HAVE_JSON
>> +static void format_qgroupid(char *buf, size_t size, u64 qgroupid)
>> +{
>> + int ret;
>> +
>> + ret = snprintf(buf, size, "%llu/%llu",
>> + btrfs_qgroup_level(qgroupid),
>> + btrfs_qgroup_subvid(qgroupid));
>> + ASSERT(ret < sizeof(buf));
>
> This is designed to catch truncated snprintf(), right?
> This can be addressed by setting up the @buf properly.
> (See below)
>
> And in fact, due to that super magic number, we won't hit this ASSERT()
> anyway.
Yep, but ASSERTs are there to detect/prevent developer mistakes. This
should only trigger due to a simple bug, so we ASSERT rather than handle
the error gracefully.
[...]
>> +static bool export_one_qgroup(json_object *container,
>> + const struct btrfs_qgroup *qgroup, bool compat)
>> +{
>> + json_object *obj = json_object_new_object();
>> + json_object *tmp;
>> + char buf[42];
>
> Answer to the ultimate question of life, the universe, and everything. :)
>
> Although according to the format level/subvolid, it should be
> count_digits(MAX_U16) + 1 + count_digits(MAX_U48) + 1. (1 for '/' and 1
> for '\n')
>
> Could be defined as a macro as:
> #define QGROUP_FORMAT_BUF_LEN (count_digits(1ULL<<16) + 1 + \
> count_digits(1ULL<<48) + 1)
Which would mean we'd execute count_digits twice for every qgroup to
resolve a constant. I'll replace the magic number with a define though.
> BTW, the result is just 22.
It's a worst-case. We're using %llu, so 42 is the length of two 64-bit
numbers in base ten, plus the slash and nul terminator. In practice we
won't hit the limit, but it doesn't hurt.
Thanks for the review.
-Jeff
--
Jeff Mahoney
SUSE Labs
signature.asc
Description: OpenPGP digital signature
