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

[Konstantin Khorenko]

On 05/12/2016 07:41 PM, Maxim Patlasov wrote:
> 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?

i just wrongly understood the phrase: "the -ENOENT error from ploop_pb_get_pending() must be interpreted as "OK, no more data present". :(
Due to some reason i thought _userspace_ should interpret it in that way, which is not true.
Sorry for the noise.

>>
>> 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
> .
>