Re: [Qemu-devel] [PATCH v2] usb-mtp: fix return status of delete

[Bandan Das]

Peter Maydell <address@hidden> writes:

> On Tue, 12 Mar 2019 at 18:25, Bandan Das <address@hidden> wrote:
>>
>>
>> Spotted by Coverity: CID 1399414
>>
>> mtp delete allows the return status of delete succeeded,
>> partial_delete or readonly - when none of the objects could be
>> deleted. Give more meaningful names to return values of the
>> delete function.
>>
>> Some initiators recurse over the objects themselves. In that case,
>> only READ_ONLY can be returned.
>>
>> Signed-off-by: Bandan Das <address@hidden>
>> ---
>> v2:
>>  Change the enum variable names and specify them as bits
>>  Add a comment describing the bit definitions
>>  Modify commit message slightly
>>
>>  hw/usb/dev-mtp.c | 62 ++++++++++++++++++++++++++----------------------
>>  1 file changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index 06e376bcd2..2b6fe77a6b 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -1135,11 +1135,19 @@ static MTPData 
>> *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c,
>>      return d;
>>  }
>>
>> -/* Return correct return code for a delete event */
>> +/*
>> + * Return values when object @o is deleted.
>> + * If at least one of the deletions succeeded,
>> + * DELETE_SUCCESS is set and if at least one
>> + * of the deletions failed, DELETE_FAILURE is
>> + * set. Both bits being set (DELETE_PARTIAL)
>> + * signifies a  RES_PARITAL_DELETE being sent
>
> "PARTIAL"
>
>> + * back to the initiator.
>> + */
>
>> +    switch (ret) {
>> +    case DELETE_SUCCESS:
>>          usb_mtp_queue_result(s, RES_OK, trans,
>>                               0, 0, 0, 0);
>> -        return;
>> +        break;
>> +    case DELETE_FAILURE:
>> +        usb_mtp_queue_result(s, RES_STORE_READ_ONLY,
>> +                             trans, 0, 0, 0, 0);
>> +        break;
>> +    case DELETE_PARTIAL:
>> +        usb_mtp_queue_result(s, RES_PARTIAL_DELETE,
>> +                             trans, 0, 0, 0, 0);
>> +        break;
>> +    default:
>> +        assert(ret != 0);
>
> You could just write the default case as:
>    default:
>        g_assert_not_reached();
>
> I think ?
>
>>      }
>
Thanks, sent a v3.

> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
>
Does the reviewed-by tag get picked up by the scripts for
a newer version of the patch or I should add it if I send one
with minor edits ?

Bandan

> thanks
> -- PMM