Re: [PATCH 8/8] btrfs-progs: qgroups: export qgroups usage information as JSON

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2478 bytes --]

On 3/7/18 1:34 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:47, jeffm@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]