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

[Peter Maydell]

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 ?

>      }

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM