[Qemu-devel] [PATCH 0/5] usb-mtp write fixes

[Qemu-devel] [PATCH 0/5] usb-mtp write fixes

[Bandan Das]

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(-)

-- 
2.14.4

[Qemu-devel] [PATCH 1/5] dev-mtp: add support for canceling transaction

[Bandan Das]

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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1ded7ac9a3..c40b0de0bb 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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)
-- 
2.14.4

[Qemu-devel] [PATCH 2/5] dev-mtp: fix buffer allocation for writing file contents

[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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index c40b0de0bb..1b72603dc5 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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;
     }
-- 
2.14.4

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

[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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1b72603dc5..c8f6eb4e9e 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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;
-- 
2.14.4

[Qemu-devel] [PATCH 4/5] dev-mtp: Add support for > 4GB file transfers

[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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index c8f6eb4e9e..2e3ea58da6 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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;
-- 
2.14.4

[Qemu-devel] [PATCH 5/5] dev-mtp: rename x-root to rootdir

[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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2e3ea58da6..3fdc4b0da1 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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(),
-- 
2.14.4

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

[Gerd Hoffmann]

On Fri, Jul 20, 2018 at 05:40:18PM -0400, Bandan Das wrote:
> 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

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

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Fri, Jul 20, 2018 at 05:40:18PM -0400, Bandan Das wrote:
>> 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.

> cheers,
>   Gerd

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

[Thomas Huth]

On 2018-07-20 23:40, Bandan Das wrote:
> 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 <bsd@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 1b72603dc5..c8f6eb4e9e 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -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)) {

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