Re: [Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects

[Bandan Das <bsd@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

>>  #define TYPE_USB_MTP "usb-mtp"
>> @@ -804,6 +815,7 @@ static MTPData *usb_mtp_get_device_info(MTPState *s, MTPControl *c)
>>          CMD_GET_OBJECT_HANDLES,
>>          CMD_GET_OBJECT_INFO,
>>          CMD_DELETE_OBJECT,
>> +        CMD_SEND_OBJECT,
>
> Seems we should not advertize this for readonly devices.

Advertising CMD_SEND_OBJECT shows that it's supported but the device is
read-only probably due to a server side setting. It differentiates between
Operation_Not_Supported and Store_Read_Only.

I agree, we should not rely on the initiator to check for this.
I will add a check for readonly when accepting DELETE, OBJECT_INFO
and return the READ_ONLY response code.

Bandan


>>          CMD_GET_OBJECT,
>>          CMD_GET_PARTIAL_OBJECT,
>>          CMD_GET_OBJECT_PROPS_SUPPORTED,
>> @@ -1378,6 +1390,14 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>>          nres = 1;
>>          res0 = data_in->length;
>>          break;
>> +    case CMD_SEND_OBJECT:
>> +        if (!s->write_pending) {
>> +            usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO,
>> +                                 c->trans, 0, 0, 0, 0);
>> +            return;
>> +        }
>> +        s->data_out = usb_mtp_data_alloc(c);
>> +        return;
>>      case CMD_GET_OBJECT_PROPS_SUPPORTED:
>>          if (c->argv[0] != FMT_UNDEFINED_OBJECT &&
>>              c->argv[0] != FMT_ASSOCIATION) {
>> @@ -1472,12 +1492,126 @@ static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
>>      fprintf(stderr, "%s\n", __func__);
>>  }
>>  
>> +static void usb_mtp_write_data(MTPState *s)
>> +{
>> +    MTPData *d = s->data_out;
>> +    MTPObject *parent =
>> +        usb_mtp_object_lookup(s, s->dataset.parent_handle);
>> +    char *path = NULL;
>> +    int rc = -1;
>> +    mode_t mask = 0644;
>> +
>> +    assert(d != NULL);
>> +
>
>
> Somewhere in here should surely be validating the "readonly" flag.
>
>> +    if (parent == NULL || !s->write_pending) {
>> +        usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
>> +                             0, 0, 0, 0);
>> +        return;
>> +    }
>> +
>
> Regards,
> Daniel