Re: [Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects
Daniel P. Berrangé 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
Re: [Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects
On Wed, Feb 21, 2018 at 12:11:00PM +0100, Gerd Hoffmann wrote:
> > > +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) {
>
> Does happens here. With a readonly device write_pending should
> never be true.
Unless I'm mis-understanding the flow, the next patch appears to set
write_pending = true, in response to a guest command, without checking
the readonly flag.
>
> > > +usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
> > > + 0, 0, 0, 0);
> > > +return;
> > > +}
>
> But adding an "assert(!readonly)" here as double-check surely doesn't hurt.
>
> cheers,
> Gerd
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects
> > +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) {
Does happens here. With a readonly device write_pending should
never be true.
> > +usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
> > + 0, 0, 0, 0);
> > +return;
> > +}
But adding an "assert(!readonly)" here as double-check surely doesn't hurt.
cheers,
Gerd
Re: [Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects
On Tue, Feb 20, 2018 at 05:59:03PM -0500, Bandan Das wrote:
> Allow write operations on behalf of the initiator. The
> precursor to write is the sending of the write metadata
> that consists of the ObjectInfo dataset. This patch introduces
> a flag that is set when the responder is ready to receive
> write data based on a previous SendObjectInfo operation by
> the initiator (The SendObjectInfo implementation is in a
> later patch)
>
> Signed-off-by: Bandan Das
> ---
> hw/usb/dev-mtp.c | 152
> ++-
> 1 file changed, 150 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 5ef77f3e9f..9b51708614 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -47,6 +47,7 @@ enum mtp_code {
> CMD_GET_OBJECT_INFO= 0x1008,
> CMD_GET_OBJECT = 0x1009,
> CMD_DELETE_OBJECT = 0x100b,
> +CMD_SEND_OBJECT= 0x100d,
> CMD_GET_PARTIAL_OBJECT = 0x101b,
> CMD_GET_OBJECT_PROPS_SUPPORTED = 0x9801,
> CMD_GET_OBJECT_PROP_DESC = 0x9802,
> @@ -63,9 +64,11 @@ enum mtp_code {
> RES_INVALID_STORAGE_ID = 0x2008,
> RES_INVALID_OBJECT_HANDLE = 0x2009,
> RES_INVALID_OBJECT_FORMAT_CODE = 0x200b,
> +RES_STORE_FULL = 0x200c,
> RES_STORE_READ_ONLY= 0x200e,
> RES_PARTIAL_DELETE = 0x2012,
> RES_SPEC_BY_FORMAT_UNSUPPORTED = 0x2014,
> +RES_INVALID_OBJECTINFO = 0x2015,
> RES_INVALID_PARENT_OBJECT = 0x201a,
> RES_INVALID_PARAMETER = 0x201d,
> RES_SESSION_ALREADY_OPEN = 0x201e,
> @@ -183,6 +186,14 @@ struct MTPState {
> int inotifyfd;
> QTAILQ_HEAD(events, MTPMonEntry) events;
> #endif
> +/* Responder is expecting a write operation */
> +bool write_pending;
> +struct {
> +uint32_t parent_handle;
> +uint16_t format;
> +uint32_t size;
> +char *filename;
> +} dataset;
> };
>
> #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.
> 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
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH v4 4/5] usb-mtp: Introduce write support for MTP objects
Allow write operations on behalf of the initiator. The
precursor to write is the sending of the write metadata
that consists of the ObjectInfo dataset. This patch introduces
a flag that is set when the responder is ready to receive
write data based on a previous SendObjectInfo operation by
the initiator (The SendObjectInfo implementation is in a
later patch)
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 152 ++-
1 file changed, 150 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 5ef77f3e9f..9b51708614 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -47,6 +47,7 @@ enum mtp_code {
CMD_GET_OBJECT_INFO= 0x1008,
CMD_GET_OBJECT = 0x1009,
CMD_DELETE_OBJECT = 0x100b,
+CMD_SEND_OBJECT= 0x100d,
CMD_GET_PARTIAL_OBJECT = 0x101b,
CMD_GET_OBJECT_PROPS_SUPPORTED = 0x9801,
CMD_GET_OBJECT_PROP_DESC = 0x9802,
@@ -63,9 +64,11 @@ enum mtp_code {
RES_INVALID_STORAGE_ID = 0x2008,
RES_INVALID_OBJECT_HANDLE = 0x2009,
RES_INVALID_OBJECT_FORMAT_CODE = 0x200b,
+RES_STORE_FULL = 0x200c,
RES_STORE_READ_ONLY= 0x200e,
RES_PARTIAL_DELETE = 0x2012,
RES_SPEC_BY_FORMAT_UNSUPPORTED = 0x2014,
+RES_INVALID_OBJECTINFO = 0x2015,
RES_INVALID_PARENT_OBJECT = 0x201a,
RES_INVALID_PARAMETER = 0x201d,
RES_SESSION_ALREADY_OPEN = 0x201e,
@@ -183,6 +186,14 @@ struct MTPState {
int inotifyfd;
QTAILQ_HEAD(events, MTPMonEntry) events;
#endif
+/* Responder is expecting a write operation */
+bool write_pending;
+struct {
+uint32_t parent_handle;
+uint16_t format;
+uint32_t size;
+char *filename;
+} dataset;
};
#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,
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);
+
+if (parent == NULL || !s->write_pending) {
+usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
+ 0, 0, 0, 0);
+return;
+}
+
+if (s->dataset.filename) {
+path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
+if (s->dataset.format == FMT_ASSOCIATION) {
+d->fd = mkdir(path, mask);
+goto free;
+}
+if (s->dataset.size < d->length) {
+usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+goto done;
+}
+d->fd = open(path, O_CREAT | O_WRONLY, mask);
+if (d->fd == -1) {
+usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+goto done;
+}
+
+/*
+ * Return success if initiator sent 0 sized data
+ */
+if (!s->dataset.size) {
+goto success;
+}
+
+rc = write(d->fd, d->data, s->dataset.size);
+if (rc == -1) {
+usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+goto done;
+}
+if (rc != s->dataset.size) {
+usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
+ 0, 0, 0, 0);
+goto done;
+}
+}
+
+success:
+usb_mtp_queue_result(s, RES_OK, d->trans,
+ 0, 0, 0, 0);
+
+done:
+/*
+ * The write dataset is kept around and freed only
+ * on success or if another write request comes in
+ */
+if (d->fd != -1) {
+close(d->fd);
+}
+free:
+g_free(s->dataset.filename);
+g_free(path);
+s->write_pending = false;
+}
+
+s
