Hi Qu,
Thanks for the review.
On 2016/12/07 12:24, Qu Wenruo wrote:
>
>
> At 12/07/2016 11:07 AM, Tsutomu Itoh wrote:
>> The 'qgroup show' command does not synchronize filesystem.
>> Therefore, 'qgroup show' may not display the correct value unless
>> synchronized with 'filesystem sync' command etc.
>>
>> So add the '--sync' and '--no-sync' options so that we can choose
>> whether or not to synchronize when executing the command.
>
> Indeed, --sync would help in a lot of cases.
>
>>
>> Signed-off-by: Tsutomu Itoh <t-i...@jp.fujitsu.com>
>> ---
>> Documentation/btrfs-qgroup.asciidoc | 5 +++++
>> cmds-qgroup.c | 24 ++++++++++++++++++++++--
>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/btrfs-qgroup.asciidoc
>> b/Documentation/btrfs-qgroup.asciidoc
>> index 438dbc7..dd133ec 100644
>> --- a/Documentation/btrfs-qgroup.asciidoc
>> +++ b/Documentation/btrfs-qgroup.asciidoc
>> @@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means
>> descending order of <attr>.
>> If no prefix is given, use ascending order by default.
>> +
>> If multiple <attr>s is given, use comma to separate.
>> ++
>> +--sync::::
>> +invoke sync before getting info.
>
> A little more words would help, to info user that qgroup will only update
> after sync.
>
>> +--no-sync::::
>> +do not invoke sync before getting info (default).
>>
>> EXIT STATUS
>> -----------
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index bc15077..ac339f3 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
>> }
>>
>> static const char * const cmd_qgroup_show_usage[] = {
>> - "btrfs qgroup show -pcreFf "
>> - "[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
>> + "btrfs qgroup show [options] <path>",
>> "Show subvolume quota groups.",
>> "-p print parent qgroup id",
>> "-c print child qgroup id",
>> @@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
>> " list qgroups sorted by specified items",
>> " you can use '+' or '-' in front of each item.",
>> " (+:ascending, -:descending, ascending default)",
>> + "--sync invoke sync before getting info",
>> + "--no-sync do not invoke sync before getting info (default)",
>
> I see you're using -Y and -N option, it's better also to show
> the short option together, if we will use that short option.
Do you think that a short option is necessary?
I thought it was not necessary...
>
>> NULL
>> };
>>
>> @@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>> u64 qgroupid;
>> int filter_flag = 0;
>> unsigned unit_mode;
>> + int sync = 0;
>>
>> struct btrfs_qgroup_comparer_set *comparer_set;
>> struct btrfs_qgroup_filter_set *filter_set;
>> @@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>> int c;
>> static const struct option long_options[] = {
>> {"sort", required_argument, NULL, 'S'},
>> + {"sync", no_argument, NULL, 'Y'},
>> + {"no-sync", no_argument, NULL, 'N'},
>
> Although I'm not sure if "-Y" and "-N" is proper here.
> IMHO, it's more like something to say all "Yes" or "No" to any interactive
> confirmation.
>
> Maybe non-charactor option will be better.
Umm, these mean s(Y)nc/(N)osync, and the user can not specify short options...
Still would you rather change to another character?
Thanks,
Tsutomu
>
> Other part looks good to me.
>
> Thanks,
> Qu
>
>> { NULL, 0, NULL, 0 }
>> };
>>
>> @@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
>> if (ret)
>> usage(cmd_qgroup_show_usage);
>> break;
>> + case 'Y':
>> + sync = 1;
>> + break;
>> + case 'N':
>> + sync = 0;
>> + break;
>> default:
>> usage(cmd_qgroup_show_usage);
>> }
>> @@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
>> return 1;
>> }
>>
>> + if (sync) {
>> + ret = ioctl(fd, BTRFS_IOC_SYNC);
>> + if (ret < 0) {
>> + error("sync ioctl failed on '%s': %s", path,
>> + strerror(errno));
>> + close_file_or_dir(fd, dirstream);
>> + goto out;
>> + }
>> + }
>> +
>> if (filter_flag) {
>> ret = lookup_path_rootid(fd, &qgroupid);
>> if (ret < 0) {
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
