Re: [PATCH 1/2] btrfs-progs: check for no result before using results

[Anand Jain <anand.jain@oracle.com>]

On 12/17/2018 02:55 PM, Nikolay Borisov wrote:
> 
> 
> On 17.12.18 г. 5:13 ч., Anand Jain wrote:
>> User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
>> using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
>> initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
>> this value in checking for the error.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   cmds-replace.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/cmds-replace.c b/cmds-replace.c
>> index b30e6c781e64..42de4de8c031 100644
>> --- a/cmds-replace.c
>> +++ b/cmds-replace.c
>> @@ -296,6 +296,8 @@ static int cmd_replace_start(int argc, char **argv)
>>   		}
>>   
>>   		if (start_args.result !=
>> +		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
>> +		    start_args.result !=
> 
> While this change is OK, it really is redundant, since we do
> IOC_DEV_REPLACE with CMD_STATUS, meaning in kernel space we always call
> btrfs_dev_replace_status which always overwrites ->result member.

> Also looking at the other 3 cmds available for this IOCTL it's always
> guaranteed for ->result to be overwritten if it executes btrfs code.

> OTOH  if the capable, memdup or an unrecognised ->cmd  is detected then
> an ordinary error code is returned, in which case the ret < 0 check
> executes and laves via "leave_with_error" label.
> 
> While your patch is OK code wise it's really a no op

  Did you miss the point that BTRFS_FS_EXCL_OP can be set by some other
  thread such as balance?.
  So in this context BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS fails to
  report BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS and as the replace thread
  moves further ahead, the BTRFS_IOCTL_DEV_REPLACE_CMD_START will know
  that BTRFS_FS_EXCL_OP is set and the kernel does not reset the
  start_args.result value set by the user land which is
  BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT.

  Besides checking for BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT is a
  right thing in general.


>>   		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>>   			error("ioctl(DEV_REPLACE_START) on '%s' returns error: %s",
>>   				path,
>>