| Message ID | 20180220225904.16129-5-bsd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <bsd@redhat.com> > --- > 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
> > +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
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
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
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; +} + +static void usb_mtp_get_data(MTPState *s, mtp_container *container, + USBPacket *p) +{ + MTPData *d = s->data_out; + uint64_t dlen; + uint32_t data_len = p->iov.size; + + if (d->first) { + /* Total length of incoming data */ + d->length = cpu_to_le32(container->length) - sizeof(mtp_container); + /* Length of data in this packet */ + data_len -= sizeof(mtp_container); + usb_mtp_realloc(d, d->length); + d->offset = 0; + d->first = false; + } + + if (d->length - d->offset > data_len) { + dlen = data_len; + } else { + dlen = d->length - d->offset; + } + + switch (d->code) { + case CMD_SEND_OBJECT: + usb_packet_copy(p, d->data + d->offset, dlen); + d->offset += dlen; + if (d->offset == d->length) { + usb_mtp_write_data(s); + usb_mtp_data_free(s->data_out); + s->data_out = NULL; + return; + } + break; + default: + p->status = USB_RET_STALL; + return; + } +} + static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p) { MTPState *s = USB_MTP(dev); MTPControl cmd; mtp_container container; uint32_t params[5]; + uint16_t container_type; int i, rc; switch (p->ep->nr) { @@ -1567,8 +1701,13 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p) p->status = USB_RET_STALL; return; } - usb_packet_copy(p, &container, sizeof(container)); - switch (le16_to_cpu(container.type)) { + if (s->data_out && !s->data_out->first) { + container_type = TYPE_DATA; + } else { + usb_packet_copy(p, &container, sizeof(container)); + container_type = le16_to_cpu(container.type); + } + switch (container_type) { case TYPE_COMMAND: if (s->data_in || s->data_out || s->result) { trace_usb_mtp_stall(s->dev.addr, "transaction inflight"); @@ -1599,6 +1738,15 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p) (cmd.argc > 4) ? cmd.argv[4] : 0); usb_mtp_command(s, &cmd); break; + case TYPE_DATA: + /* One of the previous transfers has already errored but the + * responder is still sending data associated with it + */ + if (s->result != NULL) { + return; + } + usb_mtp_get_data(s, &container, p); + break; default: /* not needed as long as the mtp device is read-only */ p->status = USB_RET_STALL;
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 <bsd@redhat.com> --- hw/usb/dev-mtp.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 2 deletions(-)