Re: [PATCH] btrfs-progs: warn users about the possible dangers of check --repair

[Nikolay Borisov <nborisov@suse.com>]

On 17.10.19 г. 4:25 ч., Anand Jain wrote:
> On 10/16/19 10:31 PM, Nikolay Borisov wrote:
>>
>>
>> On 16.10.19 г. 17:05 ч., Johannes Thumshirn wrote:
>>> The manual page of btrfsck clearly states 'btrfs check --repair' is a
>>> dangerous operation.
>>>
>>> Although this warning is in place users do not read the manual page
>>> and/or
>>> are used to the behaviour of fsck utilities which repair the filesystem,
>>> and thus potentially cause harm.
>>>
>>> Similar to 'btrfs balance' without any filters, add a warning and a
>>> countdown, so users can bail out before eventual corrupting the
>>> filesystem
>>> more than it already is.
>>>
>>> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
>>> ---
>>>   check/main.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index fd05430c1f51..acded927281a 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -9970,6 +9970,23 @@ static int cmd_check(const struct cmd_struct
>>> *cmd, int argc, char **argv)
>>>           exit(1);
>>>       }
>>>   +    if (repair) {
>>> +        int delay = 10;
>>> +        printf("WARNING:\n\n");
>>> +        printf("\tDo not use --repair unless you are advised to do
>>> so by a developer\n");
>>> +        printf("\tor an experienced user, and then only after having
>>> accepted that no\n");
>>> +        printf("\tfsck successfully repair all types of filesystem
>>> corruption. Eg.\n");
>>> +        printf("\tsome other software or hardware bugs can fatally
>>> damage a volume.\n");
>>
>> nit: The word 'other' here is redundant, no ?
>>
>>> +        printf("\tThe operation will start in %d seconds.\n", delay);
>>> +        printf("\tUse Ctrl-C to stop it.\n");
>>> +        while (delay) {
>>> +            printf("%2d", delay--);
>>> +            fflush(stdout);
>>> +            sleep(1);
>>> +        }
>>
>> That's a long winded way to have a simple for  loop that prints 10 dots,
>> 1 second apart.
> 
> 
>>  IMO a better use experience would be to ask the user to
>> confirm and if the '-f' options i passed don't bother printing the
>> warning at all.
> 
>  Agreed. -f will suffice (at least make it non-default) is a good fix.
>  But again as Qu pointed out our test cases will fail or old test case
>  with new progs will fail.

They could be adjusted accordingly to always append the -f flag when
running --repair. After all when running tests we do expect to be able
to fix everything, no ?

> 
> Thanks, Anand
> 
>>> +        printf("\nStarting repair.\n");
>>> +    }
>>> +
>>>       /*
>>>        * experimental and dangerous
>>>        */
>>>
> 
>