On 3/7/18 12:45 AM, Qu Wenruo wrote:
>
>
> On 2018年03月03日 02:47, je...@suse.com wrote:
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index 48686436..94cd0fd3 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -280,8 +280,10 @@ static const char * const cmd_qgroup_show_usage[] = {
>> " (including ancestral qgroups)",
>> "-f list all qgroups which impact the given path",
>> " (excluding ancestral qgroups)",
>> + "-P print first-level qgroups using pathname",
>> + "-v verbose, prints all nested subvolumes",
>
> Did you mean the subvolume paths of all children qgroups?
Yes. I'll make that clearer.
>> HELPINFO_UNITS_LONG,
>> - "--sort=qgroupid,rfer,excl,max_rfer,max_excl",
>> + "--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
>> " list qgroups sorted by specified items",
>> " you can use '+' or '-' in front of each item.",
>> " (+:ascending, -:descending, ascending default)",
>> diff --git a/qgroup.c b/qgroup.c
>> index 67bc0738..83918134 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>> @@ -210,8 +220,42 @@ static void print_qgroup_column_add_blank(enum
>> btrfs_qgroup_column_enum column,
>> printf(" ");
>> }
>>
>> +void print_pathname_column(struct btrfs_qgroup *qgroup, bool verbose)
>> +{
>> + struct btrfs_qgroup_list *list = NULL;
>> +
>> + fputs(" ", stdout);
>> + if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
>> + int count = 0;
>
> Newline after declaration please.
Ack.
> And declaration in if() {} block doesn't pass checkpath IIRC.
Declarations in if () {} are fine.
>> + list_for_each_entry(list, &qgroup->qgroups,
>> + next_qgroup) {
>> + if (verbose) {
>> + struct btrfs_qgroup *member = list->qgroup;
>
> Same coding style problem here.
Ack.
>> + u64 level =
>> btrfs_qgroup_level(member->qgroupid);
>> + u64 sid = btrfs_qgroup_subvid(member->qgroupid);
>> + if (count)
>> + fputs(" ", stdout);
>> + if (btrfs_qgroup_level(member->qgroupid) == 0)
>> + fputs(member->pathname, stdout);
>
> What about checking member->pathname before outputting?
> As it could be missing.
Yep.
>> +static const char *qgroup_pathname(int fd, u64 qgroupid)
>> +{
>> + struct root_info root_info;
>> + int ret;
>> + char *pathname;
>> +
>> + ret = get_subvol_info_by_rootid_fd(fd, &root_info, qgroupid);
This is a leak too. Callers are responsible for freeing the root_info
paths. With this and your review fixed, valgrind passes with 0 leaks
for btrfs qgroups show -P.
>> + if (ret)
>> + return NULL;
>> +
>> + ret = asprintf(&pathname, "%s%s",
>> + root_info.full_path[0] == '/' ? "" : "/",
>> + root_info.full_path);
>> + if (ret < 0)
>> + return NULL;
>> +
>> + return pathname;
>> +}
>> +
>> /*
>> * Lookup or insert btrfs_qgroup into qgroup_lookup.
>> *
>> @@ -588,7 +697,7 @@ static struct btrfs_qgroup *qgroup_tree_search(struct
>> qgroup_lookup *root_tree,
>> * Return the pointer to the btrfs_qgroup if found or if inserted
>> successfully.
>> * Return ERR_PTR if any error occurred.
>> */
>> -static struct btrfs_qgroup *get_or_add_qgroup(
>> +static struct btrfs_qgroup *get_or_add_qgroup(int fd,
>> struct qgroup_lookup *qgroup_lookup, u64 qgroupid)
>> {
>> struct btrfs_qgroup *bq;
>> @@ -608,6 +717,8 @@ static struct btrfs_qgroup *get_or_add_qgroup(
>> INIT_LIST_HEAD(&bq->qgroups);
>> INIT_LIST_HEAD(&bq->members);
>>
>> + bq->pathname = qgroup_pathname(fd, qgroupid);
>> +
>
> Here qgroup_pathname() will allocate memory, but no one is freeing this
> memory.
>
> The cleaner should be in __free_btrfs_qgroup() but there is no
> modification to that function.
Ack.
Thanks for the review,
-Jeff
--
Jeff Mahoney
SUSE Labs
signature.asc
Description: OpenPGP digital signature
