Hi David,
Thanks for the review.
On 2016/12/14 19:54, David Sterba wrote:
> On Wed, Dec 07, 2016 at 04:55:15PM +0900, 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.
>>
>> Signed-off-by: Tsutomu Itoh <t-i...@jp.fujitsu.com>
>> ---
>> v2: use getopt_long with enum instead of single letter (suggested by Qu)
>> ---
>> Documentation/btrfs-qgroup.asciidoc | 6 ++++++
>> cmds-qgroup.c | 33 +++++++++++++++++++++++++++++----
>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/btrfs-qgroup.asciidoc
>> b/Documentation/btrfs-qgroup.asciidoc
>> index 438dbc7..9c65795 100644
>> --- a/Documentation/btrfs-qgroup.asciidoc
>> +++ b/Documentation/btrfs-qgroup.asciidoc
>> @@ -126,6 +126,12 @@ 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::::
>> +To retrieve information after updating the status of qgroups,
>> +invoke sync before getting information.
>
> This could be more specific, that it's a filesystem sync.
>
>> +--no-sync::::
>> +Do not invoke sync before getting information (default).
>
> I'm not sure we need this option, how is it supposed to be used?
I made it to pair with --sync, but there is no use case in particular.
So, I would like to drop this with the next patch.
>
>> @@ -311,8 +313,15 @@ static int cmd_qgroup_show(int argc, char **argv)
>>
>> while (1) {
>> int c;
>> + enum {
>> + GETOPT_VAL_SORT = 256,
>> + GETOPT_VAL_SYNC,
>> + GETOPT_VAL_NO_SYNC
>> + };
>> static const struct option long_options[] = {
>> - {"sort", required_argument, NULL, 'S'},
>> + {"sort", required_argument, NULL, GETOPT_VAL_SORT},
>
> This change is unrelated to the patch, please make a separate patch for
> that.
OK. I'll separate this with the next patch.
Thanks,
Tsutomu
>
> Otherwise looks good.
>
>> + {"sync", no_argument, NULL, GETOPT_VAL_SYNC},
>> + {"no-sync", no_argument, NULL, GETOPT_VAL_NO_SYNC},
>> { NULL, 0, NULL, 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
