[Devel] [PATCH rh7] ploop: push_backup: fix ploop_push_backup_io_read()

[Maxim Patlasov]

Kostya,

On 05/12/2016 07:05 AM, Konstantin Khorenko wrote:
> On 05/12/2016 04:26 AM, Maxim Patlasov wrote:
>> ploop_push_backup_io_read() services ioctl(PLOOP_IOC_PUSH_BACKUP_IO)
>> for direction = PLOOP_READ.
>>
>> If at least one extent is successfully filled, the -ENOENT error from
>> ploop_pb_get_pending() must be interpreted as "OK, no more data 
>> present".
>>
>> https://jira.sw.ru/browse/PSBM-45000
>>
>> Signed-off-by: Maxim Patlasov <mpatlasov at virtuozzo.com>
>> ---
>>   drivers/block/ploop/dev.c |    4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
>> index 1e511d7..c103b79 100644
>> --- a/drivers/block/ploop/dev.c
>> +++ b/drivers/block/ploop/dev.c
>> @@ -4585,7 +4585,9 @@ static int ploop_push_backup_io_read(struct 
>> ploop_device *plo, unsigned long arg
>>       while (n_extents < ctl->n_extents) {
>>           cluster_t clu, len;
>>           rc = ploop_pb_get_pending(plo->pbd, &clu, &len, n_extents);
>> -        if (rc)
>> +        if (rc == -ENOENT && n_extents)
>> +            break;
>> +        else if (rc)
>>               goto io_read_done;
>>
>>           e[n_extents].clu = clu;
>>
>
> Is userspace forced to provide an empty buffer which is going to be 
> filled by ioctl(PLOOP_IOC_PUSH_BACKUP_IO)?

no

>
> If no and n_extents == 0 and ploop_pb_get_pending() returned -ENOENT 
> due to some reason,
> userspace might read garbage from n_extents (which is probably left 
> from previous iteration)
> and don't detect a failure.

If n_extents == 0 and ploop_pb_get_pending() returned -ENOENT due to 
some reason, ploop_push_backup_io_read() will return -ENOENT as well. 
So, from userspace point of view, the ioctl() failed with errno == 
ENOENT. Why do you think it won't detect failure?

>
> May be to fill struct ploop_push_backup_io_ctl in userspace data on 
> errors as well?

I think it would be bad style. Suppose you have a prototype:

int func(struct in_arg_struct *in_arg, void out_arg_struct *out_arg);

The default assumption, until the reader dive into details of 
implementation of func(), is that if func() failed (i.e. returned err != 
0), the structure referred by out_arg was not changed.

Thanks,
Maxim