Re: [PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[Jeff Mahoney]

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, >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, _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(, "%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(>qgroups);
>>  INIT_LIST_HEAD(>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

Re: [PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[Qu Wenruo]

On 2018年03月03日 02:47, je...@suse.com wrote:
> From: Jeff Mahoney 
> 
> The btrfs qgroup show command currently only exports qgroup IDs,
> forcing the user to resolve which subvolume each corresponds to.
> 
> This patch adds pathname resolution to qgroup show so that when
> the -P option is used, the last column contains the pathname of
> the root of the subvolume it describes.  In the case of nested
> qgroups, it will show the number of member qgroups or the paths
> of the members if the -v option is used.
> 
> Pathname can also be used as a sort parameter.
> 
> Signed-off-by: Jeff Mahoney 
> ---
>  Documentation/btrfs-qgroup.asciidoc |   4 +
>  cmds-qgroup.c   |  17 -
>  kerncompat.h|   1 +
>  qgroup.c| 142 
> 
>  qgroup.h|   4 +-
>  utils.c |  22 --
>  utils.h |   2 +
>  7 files changed, 166 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/btrfs-qgroup.asciidoc 
> b/Documentation/btrfs-qgroup.asciidoc
> index 3108457c..360b3269 100644
> --- a/Documentation/btrfs-qgroup.asciidoc
> +++ b/Documentation/btrfs-qgroup.asciidoc
> @@ -97,10 +97,14 @@ print child qgroup id.
>  print limit of referenced size of qgroup.
>  -e
>  print limit of exclusive size of qgroup.
> +-P
> +print pathname to the root of the subvolume managed by qgroup.  For nested 
> qgroups, the number of members will be printed unless -v is specified.
>  -F
>  list all qgroups which impact the given path(include ancestral qgroups)
>  -f
>  list all qgroups which impact the given path(exclude ancestral qgroups)
> +-v
> +Be more verbose.  Print pathnames of member qgroups when nested.
>  --raw
>  raw numbers in bytes, without the 'B' suffix.
>  --human-readable
> 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?

>   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)",
> @@ -299,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>   int filter_flag = 0;
>   unsigned unit_mode;
>   int sync = 0;
> + bool verbose = false;
>  
>   struct btrfs_qgroup_comparer_set *comparer_set;
>   struct btrfs_qgroup_filter_set *filter_set;
> @@ -316,10 +319,11 @@ static int cmd_qgroup_show(int argc, char **argv)
>   static const struct option long_options[] = {
>   {"sort", required_argument, NULL, GETOPT_VAL_SORT},
>   {"sync", no_argument, NULL, GETOPT_VAL_SYNC},
> + {"verbose", no_argument, NULL, 'v'},
>   { NULL, 0, NULL, 0 }
>   };
>  
> - c = getopt_long(argc, argv, "pcreFf", long_options, NULL);
> + c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
>   if (c < 0)
>   break;
>   switch (c) {
> @@ -327,6 +331,10 @@ static int cmd_qgroup_show(int argc, char **argv)
>   btrfs_qgroup_setup_print_column(
>   BTRFS_QGROUP_PARENT);
>   break;
> + case 'P':
> + btrfs_qgroup_setup_print_column(
> + BTRFS_QGROUP_PATHNAME);
> + break;
>   case 'c':
>   btrfs_qgroup_setup_print_column(
>   BTRFS_QGROUP_CHILD);
> @@ -354,6 +362,9 @@ static int cmd_qgroup_show(int argc, char **argv)
>   case GETOPT_VAL_SYNC:
>   sync = 1;
>   break;
> + case 'v':
> + verbose = true;
> + break;
>   default:
>   usage(cmd_qgroup_show_usage);
>   }
> @@ -394,7 +405,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>   BTRFS_QGROUP_FILTER_PARENT,
>   qgroupid);
>   }
> - ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
> + ret = btrfs_show_qgroups(fd, 

[PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[jeffm]

From: Jeff Mahoney 

The btrfs qgroup show command currently only exports qgroup IDs,
forcing the user to resolve which subvolume each corresponds to.

This patch adds pathname resolution to qgroup show so that when
the -P option is used, the last column contains the pathname of
the root of the subvolume it describes.  In the case of nested
qgroups, it will show the number of member qgroups or the paths
of the members if the -v option is used.

Pathname can also be used as a sort parameter.

Signed-off-by: Jeff Mahoney 
---
 Documentation/btrfs-qgroup.asciidoc |   4 +
 cmds-qgroup.c   |  17 -
 kerncompat.h|   1 +
 qgroup.c| 142 
 qgroup.h|   4 +-
 utils.c |  22 --
 utils.h |   2 +
 7 files changed, 166 insertions(+), 26 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc 
b/Documentation/btrfs-qgroup.asciidoc
index 3108457c..360b3269 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -97,10 +97,14 @@ print child qgroup id.
 print limit of referenced size of qgroup.
 -e
 print limit of exclusive size of qgroup.
+-P
+print pathname to the root of the subvolume managed by qgroup.  For nested 
qgroups, the number of members will be printed unless -v is specified.
 -F
 list all qgroups which impact the given path(include ancestral qgroups)
 -f
 list all qgroups which impact the given path(exclude ancestral qgroups)
+-v
+Be more verbose.  Print pathnames of member qgroups when nested.
 --raw
 raw numbers in bytes, without the 'B' suffix.
 --human-readable
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",
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)",
@@ -299,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
int filter_flag = 0;
unsigned unit_mode;
int sync = 0;
+   bool verbose = false;
 
struct btrfs_qgroup_comparer_set *comparer_set;
struct btrfs_qgroup_filter_set *filter_set;
@@ -316,10 +319,11 @@ static int cmd_qgroup_show(int argc, char **argv)
static const struct option long_options[] = {
{"sort", required_argument, NULL, GETOPT_VAL_SORT},
{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
+   {"verbose", no_argument, NULL, 'v'},
{ NULL, 0, NULL, 0 }
};
 
-   c = getopt_long(argc, argv, "pcreFf", long_options, NULL);
+   c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
if (c < 0)
break;
switch (c) {
@@ -327,6 +331,10 @@ static int cmd_qgroup_show(int argc, char **argv)
btrfs_qgroup_setup_print_column(
BTRFS_QGROUP_PARENT);
break;
+   case 'P':
+   btrfs_qgroup_setup_print_column(
+   BTRFS_QGROUP_PATHNAME);
+   break;
case 'c':
btrfs_qgroup_setup_print_column(
BTRFS_QGROUP_CHILD);
@@ -354,6 +362,9 @@ static int cmd_qgroup_show(int argc, char **argv)
case GETOPT_VAL_SYNC:
sync = 1;
break;
+   case 'v':
+   verbose = true;
+   break;
default:
usage(cmd_qgroup_show_usage);
}
@@ -394,7 +405,7 @@ static int cmd_qgroup_show(int argc, char **argv)
BTRFS_QGROUP_FILTER_PARENT,
qgroupid);
}
-   ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
+   ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
close_file_or_dir(fd, dirstream);
free(filter_set);
free(comparer_set);
diff --git a/kerncompat.h b/kerncompat.h
index fa96715f..f97495ee 100644
--- a/kerncompat.h
+++ 

[PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[jeffm]

From: Jeff Mahoney 

The btrfs qgroup show command currently only exports qgroup IDs,
forcing the user to resolve which subvolume each corresponds to.

This patch adds pathname resolution to qgroup show so that when
the -P option is used, the last column contains the pathname of
the root of the subvolume it describes.  In the case of nested
qgroups, it will show the number of member qgroups or the paths
of the members if the -v option is used.

Pathname can also be used as a sort parameter.

Signed-off-by: Jeff Mahoney 
---
 Documentation/btrfs-qgroup.asciidoc |   4 +
 cmds-qgroup.c   |  17 -
 kerncompat.h|   1 +
 qgroup.c| 142 
 qgroup.h|   4 +-
 utils.c |  22 --
 utils.h |   2 +
 7 files changed, 166 insertions(+), 26 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc 
b/Documentation/btrfs-qgroup.asciidoc
index 3108457c..360b3269 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -97,10 +97,14 @@ print child qgroup id.
 print limit of referenced size of qgroup.
 -e
 print limit of exclusive size of qgroup.
+-P
+print pathname to the root of the subvolume managed by qgroup.  For nested 
qgroups, the number of members will be printed unless -v is specified.
 -F
 list all qgroups which impact the given path(include ancestral qgroups)
 -f
 list all qgroups which impact the given path(exclude ancestral qgroups)
+-v
+Be more verbose.  Print pathnames of member qgroups when nested.
 --raw
 raw numbers in bytes, without the 'B' suffix.
 --human-readable
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",
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)",
@@ -299,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
int filter_flag = 0;
unsigned unit_mode;
int sync = 0;
+   bool verbose = false;
 
struct btrfs_qgroup_comparer_set *comparer_set;
struct btrfs_qgroup_filter_set *filter_set;
@@ -316,10 +319,11 @@ static int cmd_qgroup_show(int argc, char **argv)
static const struct option long_options[] = {
{"sort", required_argument, NULL, GETOPT_VAL_SORT},
{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
+   {"verbose", no_argument, NULL, 'v'},
{ NULL, 0, NULL, 0 }
};
 
-   c = getopt_long(argc, argv, "pcreFf", long_options, NULL);
+   c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
if (c < 0)
break;
switch (c) {
@@ -327,6 +331,10 @@ static int cmd_qgroup_show(int argc, char **argv)
btrfs_qgroup_setup_print_column(
BTRFS_QGROUP_PARENT);
break;
+   case 'P':
+   btrfs_qgroup_setup_print_column(
+   BTRFS_QGROUP_PATHNAME);
+   break;
case 'c':
btrfs_qgroup_setup_print_column(
BTRFS_QGROUP_CHILD);
@@ -354,6 +362,9 @@ static int cmd_qgroup_show(int argc, char **argv)
case GETOPT_VAL_SYNC:
sync = 1;
break;
+   case 'v':
+   verbose = true;
+   break;
default:
usage(cmd_qgroup_show_usage);
}
@@ -394,7 +405,7 @@ static int cmd_qgroup_show(int argc, char **argv)
BTRFS_QGROUP_FILTER_PARENT,
qgroupid);
}
-   ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
+   ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
close_file_or_dir(fd, dirstream);
free(filter_set);
free(comparer_set);
diff --git a/kerncompat.h b/kerncompat.h
index fa96715f..f97495ee 100644
--- a/kerncompat.h
+++