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