usb-mtp write fixes

PATCH In review Bandan Das 3 participants 9 messages Jul 20, 2018 – Oct 19, 2018

Share & tools

Bandan Das

author

2018-07-20 21:40 UTC #0 lore raw

cover usb-mtp write fixes

Patch 1 adds support for canceling an ongoing transaction. 2,3 and 4 fix writes for large transfers. For > 4G file transfers, the logic has been modified to check for the end of the data phase by checking for a short packet. Patch 5 renames x-root to a more meaningful rootdir.

Bandan Das (5):
dev-mtp: add support for canceling transaction
dev-mtp: fix buffer allocation for writing file contents
dev-mtp: retry write for incomplete transfers
dev-mtp: Add support for > 4GB file transfers
dev-mtp: rename x-root to rootdir

 hw/usb/dev-mtp.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 79 insertions(+), 14 deletions(-)

Signature · 1 line

2.14.4

Bandan Das

author

2018-07-20 21:40 UTC #1 lore raw

PATCH 1/5 dev-mtp: add support for canceling transaction

The initiator can choose to cancel an ongoing request which is specified by bRequest=0x64. If such a request arrives, free up any pending state

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

hw/usb/dev-mtp.c +26 -4

index 1ded7ac9a3..c40b0de0bb 100644
@@ -82,6 +82,7 @@ enum mtp_code {
     FMT_ASSOCIATION                = 0x3001,
 
     /* event codes */
+    EVT_CANCEL_TRANSACTION         = 0x4001,
     EVT_OBJ_ADDED                  = 0x4002,
     EVT_OBJ_REMOVED                = 0x4003,
     EVT_OBJ_INFO_CHANGED           = 0x4007,

@@ -1551,14 +1552,35 @@ static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p,
                                    int length, uint8_t *data)
 {
     int ret;
+    MTPState *s = USB_MTP(dev);
+    uint16_t *event = (uint16_t *)data;
 
-    ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
-    if (ret >= 0) {
-        return;
+    switch (request) {
+    case ClassInterfaceOutRequest | 0x64:
+        if (*event == EVT_CANCEL_TRANSACTION) {
+            g_free(s->result);
+            s->result = NULL;
+            usb_mtp_data_free(s->data_in);
+            s->data_in = NULL;
+            if (s->write_pending) {
+                g_free(s->dataset.filename);
+                s->write_pending = false;
+            }
+            usb_mtp_data_free(s->data_out);
+            s->data_out = NULL;
+        } else {
+            p->status = USB_RET_STALL;
+        }
+        break;
+    default:
+        ret = usb_desc_handle_control(dev, p, request,
+                                      value, index, length, data);
+        if (ret >= 0) {
+            return;
+        }
     }
 
     trace_usb_mtp_stall(dev->addr, "unknown control request");
-    p->status = USB_RET_STALL;
 }
 
 static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)

Signature · 1 line

2.14.4

Bandan Das

author

2018-07-20 21:40 UTC #2 lore raw

PATCH 2/5 dev-mtp: fix buffer allocation for writing file contents

In reply to Bandan Das

usb_mtp_realloc() was being incorrectly used when allocating buffer for incoming data. Set d->length only after resizing the buffer.

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/usb/dev-mtp.c +4 -2

index c40b0de0bb..1b72603dc5 100644
@@ -1721,6 +1721,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     MTPData *d = s->data_out;
     uint64_t dlen;
     uint32_t data_len = p->iov.size;
+    uint64_t total_len;
 
     if (!d) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, 0,

@@ -1729,10 +1730,11 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     }
     if (d->first) {
         /* Total length of incoming data */
-        d->length = cpu_to_le32(container->length) - sizeof(mtp_container);
+        total_len = 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);
+        usb_mtp_realloc(d, total_len);
+        d->length += total_len;
         d->offset = 0;
         d->first = false;
     }

Signature · 1 line

2.14.4

Bandan Das

author

2018-07-20 21:40 UTC #3 lore raw

PATCH 3/5 dev-mtp: retry write for incomplete transfers

In reply to Bandan Das

For large buffers, write may not copy the full buffer. For example, on Linux, write imposes a limit of 0x7ffff000. Note that this does not fix >4G transfers but ~>2G files will transfer successfully.

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

hw/usb/dev-mtp.c +20 -2

index 1b72603dc5..c8f6eb4e9e 100644
@@ -1602,6 +1602,24 @@ static void utf16_to_str(uint8_t len, uint16_t *arr, char *name)
     g_free(wstr);
 }
 
+/* Wrapper around write, returns 0 on failure */
+static uint64_t write_retry(int fd, void *buf, uint64_t size)
+{
+        uint64_t bytes_left = size, ret;
+
+        while (bytes_left > 0) {
+                ret = write(fd, buf, bytes_left);
+                if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
+                                    errno != EWOULDBLOCK)) {
+                        break;
+                }
+                bytes_left -= ret;
+                buf += ret;
+        }
+
+        return size - bytes_left;
+}
+
 static void usb_mtp_write_data(MTPState *s)
 {
     MTPData *d = s->data_out;

@@ -1644,8 +1662,8 @@ static void usb_mtp_write_data(MTPState *s)
             goto success;
         }
 
-        rc = write(d->fd, d->data, s->dataset.size);
-        if (rc == -1) {
+        rc = write_retry(d->fd, d->data, s->dataset.size);
+        if (!rc) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;

Signature · 1 line

2.14.4

Gerd Hoffmann

2018-08-07 13:28 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 3/5] dev-mtp: retry write for incomplete transfers

Quoted 3 lines · Bandan Das

For large buffers, write may not copy the full buffer. For example, on Linux, write imposes a limit of 0x7ffff000. Note that this does not fix >4G transfers but ~>2G files will transfer successfully.

Hmm, I guess it would be a good idea to write the file in smaller pieces, so we don't need a 2G host buffer to let the guest write a 2G file ...

(as incremental improvement on top of this series).

cheers,
Gerd

Bandan Das

author

2018-08-07 18:15 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 3/5] dev-mtp: retry write for incomplete transfers

Quoted 11 lines · Gerd Hoffmann

Quoted 3 lines · Bandan Das
For large buffers, write may not copy the full buffer. For example, on Linux, write imposes a limit of 0x7ffff000. Note that this does not fix >4G transfers but ~>2G files will transfer successfully.
Hmm, I guess it would be a good idea to write the file in smaller pieces, so we don't need a 2G host buffer to let the guest write a 2G file ...
(as incremental improvement on top of this series).

Sounds good, I will add this to my todo list.

Quoted 2 lines

cheers,
Gerd

Thomas Huth

2018-10-19 09:48 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 3/5] dev-mtp: retry write for incomplete transfers

In reply to Bandan Das

Quoted 16 lines · Bandan Das

For large buffers, write may not copy the full buffer. For example, on Linux, write imposes a limit of 0x7ffff000. Note that this does not fix >4G transfers but ~>2G files will transfer successfully.
Signed-off-by Bandan Das <[email protected]>
---
 hw/usb/dev-mtp.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
hw/usb/dev-mtp.c +0 -0
index 1b72603dc5..c8f6eb4e9e 100644
@@ -1602,6 +1602,24 @@ static void utf16_to_str(uint8_t len, uint16_t *arr, char *name)
     g_free(wstr);
 }

hw/usb/dev-mtp.c static void utf16_to_str(uint8_t len, uint16_t *arr, char *name) · line 1613

Quoted 10 lines

hw/usb/dev-mtp.c +9 -0

 
+/* Wrapper around write, returns 0 on failure */
+static uint64_t write_retry(int fd, void *buf, uint64_t size)
+{
+        uint64_t bytes_left = size, ret;
+
+        while (bytes_left > 0) {
+                ret = write(fd, buf, bytes_left);
+                if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
+                                    errno != EWOULDBLOCK)) {

Someone opened a bug ticket about this here:

https://bugs.launchpad.net/qemu/+bug/1798780

The check looks wrong, indeed - either "!=" should be "==" or "||" should be "&&" here ?

Thomas

Bandan Das

author

2018-07-20 21:40 UTC #7 lore raw

PATCH 4/5 dev-mtp: Add support for > 4GB file transfers

In reply to Bandan Das

To support larger file transfers, rely on a short packet to detect end of the data phase and rewrite d->length to the size received

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

hw/usb/dev-mtp.c +27 -4

index c8f6eb4e9e..2e3ea58da6 100644
@@ -147,9 +147,12 @@ struct MTPData {
     uint32_t     trans;
     uint64_t     offset;
     uint64_t     length;
-    uint32_t     alloc;
+    uint64_t     alloc;
     uint8_t      *data;
     bool         first;
+    /* Used for >4G file sizes */
+    bool         pending;
+    uint64_t     cached_length;
     int          fd;
 };

@@ -1626,7 +1629,7 @@ static void usb_mtp_write_data(MTPState *s)
     MTPObject *parent =
         usb_mtp_object_lookup(s, s->dataset.parent_handle);
     char *path = NULL;
-    int rc = -1;
+    uint64_t rc;
     mode_t mask = 0644;
 
     assert(d != NULL);

@@ -1643,7 +1646,7 @@ static void usb_mtp_write_data(MTPState *s)
             d->fd = mkdir(path, mask);
             goto free;
         }
-        if (s->dataset.size < d->length) {
+        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size < d->length)) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;

@@ -1754,13 +1757,27 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         usb_mtp_realloc(d, total_len);
         d->length += total_len;
         d->offset = 0;
+        d->cached_length = total_len;
         d->first = false;
+        d->pending = false;
+    }
+
+    if (d->pending) {
+        usb_mtp_realloc(d, d->cached_length);
+        d->length += d->cached_length;
+        d->pending = false;
     }
 
     if (d->length - d->offset > data_len) {
         dlen = data_len;
     } else {
         dlen = d->length - d->offset;
+        /* Check for cached data for large files */
+        if ((s->dataset.size == 0xFFFFFFFF) && (dlen < p->iov.size)) {
+            usb_mtp_realloc(d, p->iov.size - dlen);
+            d->length += p->iov.size - dlen;
+            dlen = p->iov.size;
+        }
     }
 
     switch (d->code) {

@@ -1780,12 +1797,18 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     case CMD_SEND_OBJECT:
         usb_packet_copy(p, d->data + d->offset, dlen);
         d->offset += dlen;
-        if (d->offset == d->length) {
+        if ((p->iov.size % 64) || !p->iov.size) {
+            assert((s->dataset.size == 0xFFFFFFFF) ||
+                   (s->dataset.size == d->length));
+
             usb_mtp_write_data(s);
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;
             return;
         }
+        if (d->offset == d->length) {
+            d->pending = true;
+        }
         break;
     default:
         p->status = USB_RET_STALL;

Signature · 1 line

2.14.4

Bandan Das

author

2018-07-20 21:40 UTC #8 lore raw

PATCH 5/5 dev-mtp: rename x-root to rootdir

In reply to Bandan Das

x-root was renamed as such owing to the experimental nature of the property; the underlying filesystem semantics were undecided

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/usb/dev-mtp.c +2 -2

index 2e3ea58da6..3fdc4b0da1 100644
@@ -2018,7 +2018,7 @@ static void usb_mtp_realize(USBDevice *dev, Error **errp)
     QTAILQ_INIT(&s->objects);
     if (s->desc == NULL) {
         if (s->root == NULL) {
-            error_setg(errp, "usb-mtp: x-root property must be configured");
+            error_setg(errp, "usb-mtp: rootdir property must be configured");
             return;
         }
         s->desc = strrchr(s->root, '/');

@@ -2047,7 +2047,7 @@ static const VMStateDescription vmstate_usb_mtp = {
 };
 
 static Property mtp_properties[] = {
-    DEFINE_PROP_STRING("x-root", MTPState, root),
+    DEFINE_PROP_STRING("rootdir", MTPState, root),
     DEFINE_PROP_STRING("desc", MTPState, desc),
     DEFINE_PROP_BOOL("readonly", MTPState, readonly, true),
     DEFINE_PROP_END_OF_LIST(),

Signature · 1 line

2.14.4