[Qemu-devel] [PULL 0/8] Usb 20190130 patches

[Qemu-devel] [PULL 0/8] Usb 20190130 patches

[Gerd Hoffmann]

The following changes since commit b4fbe1f65a4769c09e6bf2d79fc84360f840f40e:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190129' into staging (2019-01-29 12:00:19 +0000)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/usb-20190130-pull-request

for you to fetch changes up to 49f9e8d660d41cbe325d554e742d7100d59a7abf:

  usb-mtp: replace the homebrew write with qemu_write_full (2019-01-30 06:47:52 +0100)

----------------------------------------------------------------
usb: xhci: fix iso transfers.
usb: mtp: break up writes, bugfixes.
usb: fix lgpl info in headers.
usb: hid: unique serials.

----------------------------------------------------------------

Bandan Das (3):
  usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ
  usb-mtp: breakup MTP write into smaller chunks
  usb-mtp: replace the homebrew write with qemu_write_full

Gerd Hoffmann (1):
  usb: assign unique serial numbers to hid devices

Li Qiang (1):
  usb: dev-mtp: close fd in usb_mtp_object_readdir()

Thomas Huth (1):
  hw/usb: Fix LGPL information in the file headers

Yuri Benditovich (2):
  usb: XHCI shall not halt isochronous endpoints
  usb: implement XHCI underrun/overrun events

 hw/usb/hcd-ehci.h        |   4 +-
 hw/usb/hcd-xhci.h        |   1 +
 hw/core/machine.c        |   3 +
 hw/usb/combined-packet.c |   4 +-
 hw/usb/dev-hid.c         |  26 ++++----
 hw/usb/dev-mtp.c         | 168 +++++++++++++++++++++++++++++------------------
 hw/usb/hcd-ehci-pci.c    |   4 +-
 hw/usb/hcd-ehci-sysbus.c |   4 +-
 hw/usb/hcd-ehci.c        |   5 +-
 hw/usb/hcd-xhci.c        |  22 ++++++-
 10 files changed, 153 insertions(+), 88 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/8] usb: assign unique serial numbers to hid devices

[Gerd Hoffmann]

Windows guests have trouble dealing with usb devices having identical
serial numbers.  So, assign unique serial numbers to usb hid devices.
All other usb devices have this already.

In the past the fixed serial number has been used to indicate working
remote setup to linux guests.  Here is a bit of history:

 * First there was nothing.
 * Then I added a rule to udev checking for serial == 42.
   (this is in rhel-6).
 * Then systemd + udev merged.
 * Then I changed the rule to check for serial != 1 instead, so we can
   use any serial but "1" which is the one the old broken devices had
   (this is in rhel-7).  March 2014 in upstream systemd.
 * Then all usb power management rules where dropped from systemd (June
   2015).  Which I figured today (Sept 2018), after wondering that the
   rules are gone in fedora 28.

So, three years ago the serial number check was dropped upstream, yet I
hav't seen a single report about autosuspend issues (or cpu usage for
usb emulation going up, which is the typical symtom).

So I figured I can stop worring that changing the serial number will
break things and just do it.

And even if it turns out autosuspend is still an issue:  I think
meanwhile we can really stop worrying about guests running in old qemu
versions with broken usb suspend (fixed in 0.13 !).  If needed we can
enable autosuspend unconditionally in guests.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20190110125108.22834-1-kraxel@redhat.com
---
 hw/core/machine.c |  3 +++
 hw/usb/dev-hid.c  | 26 +++++++++++++++-----------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2629515363..077fbd182a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,9 @@ GlobalProperty hw_compat_3_1[] = {
     { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" },
     { "tpm-crb", "ppi", "false" },
     { "tpm-tis", "ppi", "false" },
+    { "usb-kbd", "serial", "42" },
+    { "usb-mouse", "serial", "42" },
+    { "usb-kbd", "serial", "42" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 90cd745f06..f9ea3033a1 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -61,10 +61,13 @@ enum {
     STR_PRODUCT_MOUSE,
     STR_PRODUCT_TABLET,
     STR_PRODUCT_KEYBOARD,
-    STR_SERIALNUMBER,
+    STR_SERIAL_COMPAT,
     STR_CONFIG_MOUSE,
     STR_CONFIG_TABLET,
     STR_CONFIG_KEYBOARD,
+    STR_SERIAL_MOUSE,
+    STR_SERIAL_TABLET,
+    STR_SERIAL_KEYBOARD,
 };
 
 static const USBDescStrings desc_strings = {
@@ -72,10 +75,13 @@ static const USBDescStrings desc_strings = {
     [STR_PRODUCT_MOUSE]    = "QEMU USB Mouse",
     [STR_PRODUCT_TABLET]   = "QEMU USB Tablet",
     [STR_PRODUCT_KEYBOARD] = "QEMU USB Keyboard",
-    [STR_SERIALNUMBER]     = "42", /* == remote wakeup works */
+    [STR_SERIAL_COMPAT]    = "42",
     [STR_CONFIG_MOUSE]     = "HID Mouse",
     [STR_CONFIG_TABLET]    = "HID Tablet",
     [STR_CONFIG_KEYBOARD]  = "HID Keyboard",
+    [STR_SERIAL_MOUSE]     = "89126",
+    [STR_SERIAL_TABLET]    = "28754",
+    [STR_SERIAL_KEYBOARD]  = "68284",
 };
 
 static const USBDescIface desc_iface_mouse = {
@@ -375,7 +381,7 @@ static const USBDesc desc_mouse = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_MOUSE,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_MOUSE,
     },
     .full = &desc_device_mouse,
     .str  = desc_strings,
@@ -389,7 +395,7 @@ static const USBDesc desc_mouse2 = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_MOUSE,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_MOUSE,
     },
     .full = &desc_device_mouse,
     .high = &desc_device_mouse2,
@@ -404,7 +410,7 @@ static const USBDesc desc_tablet = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_TABLET,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_TABLET,
     },
     .full = &desc_device_tablet,
     .str  = desc_strings,
@@ -418,7 +424,7 @@ static const USBDesc desc_tablet2 = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_TABLET,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_TABLET,
     },
     .full = &desc_device_tablet,
     .high = &desc_device_tablet2,
@@ -433,7 +439,7 @@ static const USBDesc desc_keyboard = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_KEYBOARD,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_KEYBOARD,
     },
     .full = &desc_device_keyboard,
     .str  = desc_strings,
@@ -447,7 +453,7 @@ static const USBDesc desc_keyboard2 = {
         .bcdDevice         = 0,
         .iManufacturer     = STR_MANUFACTURER,
         .iProduct          = STR_PRODUCT_KEYBOARD,
-        .iSerialNumber     = STR_SERIALNUMBER,
+        .iSerialNumber     = STR_SERIAL_KEYBOARD,
     },
     .full = &desc_device_keyboard,
     .high = &desc_device_keyboard2,
@@ -718,9 +724,7 @@ static void usb_hid_initfn(USBDevice *dev, int kind,
         return;
     }
 
-    if (dev->serial) {
-        usb_desc_set_string(dev, STR_SERIALNUMBER, dev->serial);
-    }
+    usb_desc_create_serial(dev);
     usb_desc_init(dev);
     us->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
     hid_init(&us->hid, kind, usb_hid_changed);
-- 
2.9.3

[Qemu-devel] [PULL 2/8] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Gerd Hoffmann]

From: Li Qiang <liq3ea@163.com>

Spotted by Coverity: CID 1397070

Signed-off-by: Li Qiang <liq3ea@163.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20190103133113.49599-1-liq3ea@163.com

[ kraxel: dropped chunk which adds close() after successful
          fdopendir() call, that is not needed according to
          POSIX even though Coverity flags it as bug ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 68c5eb8eaa..837c9d9449 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -666,6 +666,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
     }
     dir = fdopendir(fd);
     if (!dir) {
+        close(fd);
         return;
     }
 #ifdef CONFIG_INOTIFY1
-- 
2.9.3

[Qemu-devel] [PULL 3/8] hw/usb: Fix LGPL information in the file headers

[Gerd Hoffmann]

From: Thomas Huth <thuth@redhat.com>

It's either "GNU *Library* General Public version 2" or "GNU Lesser
General Public version *2.1*", but there was no "version 2.0" of the
"Lesser" library. So assume that version 2.1 is meant here.
Additionally, suggest that the user should have received a copy of
the LGPL, and not the GPL here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 1548254454-7659-1-git-send-email-thuth@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.h        | 4 ++--
 hw/usb/combined-packet.c | 4 ++--
 hw/usb/hcd-ehci-pci.c    | 4 ++--
 hw/usb/hcd-ehci-sysbus.c | 4 ++--
 hw/usb/hcd-ehci.c        | 5 ++---
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index d6601706ee..fedf82c611 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -4,14 +4,14 @@
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
- * version 2 of the License, or(at your option) any later version.
+ * version 2.1 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index fc98383d30..37b23e20ef 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -9,14 +9,14 @@
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
- * version 2 of the License, or(at your option) any later version.
+ * version 2.1 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 69abbf7b7b..38b24b160a 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -4,14 +4,14 @@
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
- * version 2 of the License, or(at your option) any later version.
+ * version 2.1 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 331faf8bc3..9f7f128f19 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -4,14 +4,14 @@
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
- * version 2 of the License, or(at your option) any later version.
+ * version 2.1 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index e233681962..9b132cb0d3 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -12,18 +12,17 @@
  * Niels de Vos.  David S. Ahern continued working on it.  Kevin Wolf,
  * Jan Kiszka and Vincent Palatin contributed bugfixes.
  *
- *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
- * version 2 of the License, or(at your option) any later version.
+ * version 2.1 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-- 
2.9.3

[Qemu-devel] [PULL 4/8] usb: XHCI shall not halt isochronous endpoints

[Gerd Hoffmann]

From: Yuri Benditovich <yuri.benditovich@janustech.com>

According to the XHCI spec (4.10.2) the controller
never halts isochronous endpoints. This commit prevent
stop of isochronous streaming when sporadic errors
status received from backends.

Signed-off-by: Yuri Benditovich <yuri.benditovich@janustech.com>
Message-id: 20190128200444.5128-2-yuri.benditovich@janustech.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 8f1a01a405..1a8fd9644e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1571,6 +1571,11 @@ static void xhci_stall_ep(XHCITransfer *xfer)
     uint32_t err;
     XHCIStreamContext *sctx;
 
+    if (epctx->type == ET_ISO_IN || epctx->type == ET_ISO_OUT) {
+        /* never halt isoch endpoints, 4.10.2 */
+        return;
+    }
+
     if (epctx->nr_pstreams) {
         sctx = xhci_find_stream(epctx, xfer->streamid, &err);
         if (sctx == NULL) {
-- 
2.9.3

[Qemu-devel] [PULL 5/8] usb: implement XHCI underrun/overrun events

[Gerd Hoffmann]

From: Yuri Benditovich <yuri.benditovich@janustech.com>

Implement underrun/overrun events of isochronous endpoints
according to XHCI spec (4.10.3.1)
Guest software restarts data streaming when receives these events.
The XHCI reports these events using interrupter assigned
to the slot (as these events do not have TRB), so current
commit adds the field of assigned interrupter to the
XHCISlot structure. Guest software assigns interrupter to the
slot on 'Address Device' and 'Evaluate Context' commands.

Signed-off-by: Yuri Benditovich <yuri.benditovich@janustech.com>
Message-id: 20190128200444.5128-3-yuri.benditovich@janustech.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.h |  1 +
 hw/usb/hcd-xhci.c | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index fc36a4c787..240caa4e51 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -140,6 +140,7 @@ typedef struct XHCIPort {
 typedef struct XHCISlot {
     bool enabled;
     bool addressed;
+    uint16_t intr;
     dma_addr_t ctx;
     USBPort *uport;
     XHCIEPContext *eps[31];
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 1a8fd9644e..19c64f7ff4 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1949,6 +1949,16 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
     while (1) {
         length = xhci_ring_chain_length(xhci, ring);
         if (length <= 0) {
+            if (epctx->type == ET_ISO_OUT || epctx->type == ET_ISO_IN) {
+                /* 4.10.3.1 */
+                XHCIEvent ev = { ER_TRANSFER };
+                ev.ccode  = epctx->type == ET_ISO_IN ?
+                    CC_RING_OVERRUN : CC_RING_UNDERRUN;
+                ev.slotid = epctx->slotid;
+                ev.epid   = epctx->epid;
+                ev.ptr    = epctx->ring.dequeue;
+                xhci_event(xhci, &ev, xhci->slots[epctx->slotid-1].intr);
+            }
             break;
         }
         xfer = xhci_ep_alloc_xfer(epctx, length);
@@ -2028,6 +2038,7 @@ static TRBCCode xhci_disable_slot(XHCIState *xhci, unsigned int slotid)
     xhci->slots[slotid-1].enabled = 0;
     xhci->slots[slotid-1].addressed = 0;
     xhci->slots[slotid-1].uport = NULL;
+    xhci->slots[slotid-1].intr = 0;
     return CC_SUCCESS;
 }
 
@@ -2127,6 +2138,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
     slot = &xhci->slots[slotid-1];
     slot->uport = uport;
     slot->ctx = octx;
+    slot->intr = get_field(slot_ctx[2], TRB_INTR);
 
     /* Make sure device is in USB_STATE_DEFAULT state */
     usb_device_reset(dev);
@@ -2300,8 +2312,9 @@ static TRBCCode xhci_evaluate_slot(XHCIState *xhci, unsigned int slotid,
 
         slot_ctx[1] &= ~0xFFFF; /* max exit latency */
         slot_ctx[1] |= islot_ctx[1] & 0xFFFF;
-        slot_ctx[2] &= ~0xFF00000; /* interrupter target */
-        slot_ctx[2] |= islot_ctx[2] & 0xFF000000;
+        /* update interrupter target field */
+        xhci->slots[slotid-1].intr = get_field(islot_ctx[2], TRB_INTR);
+        set_field(&slot_ctx[2], xhci->slots[slotid-1].intr, TRB_INTR);
 
         DPRINTF("xhci: output slot context: %08x %08x %08x %08x\n",
                 slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]);
-- 
2.9.3

[Qemu-devel] [PULL 6/8] usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ

[Gerd Hoffmann]

From: Bandan Das <bsd@redhat.com>

This is a "pre-patch" to breaking up the write buffer for
MTP writes. Instead of allocating a mtp buffer equal to size
sent by the initiator, we start with a small size and reallocate
multiples (of that small size) as needed.

Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20190129131908.27924-2-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 837c9d9449..2e342c7745 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -25,6 +25,7 @@
 #include "trace.h"
 #include "hw/usb.h"
 #include "desc.h"
+#include "qemu/units.h"
 
 /* ----------------------------------------------------------------------- */
 
@@ -152,7 +153,6 @@ struct MTPData {
     bool         first;
     /* Used for >4G file sizes */
     bool         pending;
-    uint64_t     cached_length;
     int          fd;
 };
 
@@ -244,6 +244,7 @@ typedef struct {
 
 #define MTP_MANUFACTURER  "QEMU"
 #define MTP_PRODUCT       "QEMU filesharing"
+#define MTP_WRITE_BUF_SZ  (512 * KiB)
 
 enum {
     STR_MANUFACTURER = 1,
@@ -1659,7 +1660,7 @@ static void usb_mtp_write_data(MTPState *s)
             d->fd = mkdir(path, mask);
             goto free;
         }
-        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size < d->length)) {
+        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size != d->offset)) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;
@@ -1777,17 +1778,21 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *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, total_len);
-        d->length += total_len;
+        if (total_len < MTP_WRITE_BUF_SZ) {
+                usb_mtp_realloc(d, total_len);
+                d->length += total_len;
+        } else {
+                usb_mtp_realloc(d, MTP_WRITE_BUF_SZ - sizeof(mtp_container));
+                d->length += MTP_WRITE_BUF_SZ - sizeof(mtp_container);
+        }
         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;
+        usb_mtp_realloc(d, MTP_WRITE_BUF_SZ);
+        d->length += MTP_WRITE_BUF_SZ;
         d->pending = false;
     }
 
@@ -1795,12 +1800,6 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         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) {
@@ -1822,7 +1821,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         d->offset += dlen;
         if ((p->iov.size % 64) || !p->iov.size) {
             assert((s->dataset.size == 0xFFFFFFFF) ||
-                   (s->dataset.size == d->length));
+                   (s->dataset.size == d->offset));
 
             usb_mtp_write_data(s);
             usb_mtp_data_free(s->data_out);
-- 
2.9.3

[Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Gerd Hoffmann]

From: Bandan Das <bsd@redhat.com>

For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
getting the next block of data. The file is kept opened for the
duration of the operation but the sanity checks on the write operation
are performed only once when the write operation starts. Additionally,
we also update the file size in the object metadata once the file has
completely been written.

Suggested-by: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20190129131908.27924-3-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 134 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 91 insertions(+), 43 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2e342c7745..2ca25c9da5 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -36,6 +36,13 @@ enum mtp_container_type {
     TYPE_EVENT    = 4,
 };
 
+/* MTP write stage, for internal use only */
+enum mtp_write_status {
+    WRITE_START    = 1,
+    WRITE_CONTINUE = 2,
+    WRITE_END      = 3,
+};
+
 enum mtp_code {
     /* command codes */
     CMD_GET_DEVICE_INFO            = 0x1001,
@@ -154,6 +161,9 @@ struct MTPData {
     /* Used for >4G file sizes */
     bool         pending;
     int          fd;
+    uint8_t      write_status;
+    /* Internal pointer per every MTP_WRITE_BUF_SZ */
+    uint64_t     data_offset;
 };
 
 struct MTPObject {
@@ -1620,10 +1630,14 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
 }
 
 /* Wrapper around write, returns 0 on failure */
-static uint64_t write_retry(int fd, void *buf, uint64_t size)
+static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
 {
         uint64_t bytes_left = size, ret;
 
+        if (lseek(fd, offset, SEEK_SET) < 0) {
+            goto done;
+        }
+
         while (bytes_left > 0) {
                 ret = write(fd, buf, bytes_left);
                 if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
@@ -1634,9 +1648,20 @@ static uint64_t write_retry(int fd, void *buf, uint64_t size)
                 buf += ret;
         }
 
+done:
         return size - bytes_left;
 }
 
+static void usb_mtp_update_object(MTPObject *parent, char *name)
+{
+    MTPObject *o =
+        usb_mtp_object_lookup_name(parent, name, strlen(name));
+
+    if (o) {
+        lstat(o->path, &o->stat);
+    }
+}
+
 static void usb_mtp_write_data(MTPState *s)
 {
     MTPData *d = s->data_out;
@@ -1648,48 +1673,56 @@ static void usb_mtp_write_data(MTPState *s)
 
     assert(d != NULL);
 
-    if (parent == NULL || !s->write_pending) {
-        usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
-                             0, 0, 0, 0);
+    switch (d->write_status) {
+    case WRITE_START:
+        if (!parent || !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 != 0xFFFFFFFF) && (s->dataset.size != d->offset)) {
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
-            goto done;
-        }
-        d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, 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;
-        }
+        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;
+            }
+            d->fd = open(path, O_CREAT | O_WRONLY |
+                         O_CLOEXEC | O_NOFOLLOW, mask);
+            if (d->fd == -1) {
+                usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+                                     0, 0, 0, 0);
+                goto done;
+            }
 
-        rc = write_retry(d->fd, d->data, d->offset);
-        if (rc != d->offset) {
+            /* Return success if initiator sent 0 sized data */
+            if (!s->dataset.size) {
+                goto success;
+            }
+            if (d->length != MTP_WRITE_BUF_SZ && !d->pending) {
+                d->write_status = WRITE_END;
+            }
+        }
+        /* fall through */
+    case WRITE_CONTINUE:
+    case WRITE_END:
+        rc = write_retry(d->fd, d->data, d->data_offset,
+                         d->offset - d->data_offset);
+        if (rc != d->data_offset) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;
+        }
+        if (d->write_status != WRITE_END) {
+            return;
+        } else {
+            /* Only for < 4G file sizes */
+            if (s->dataset.size != 0xFFFFFFFF && d->offset != s->dataset.size) {
+                usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
+                                     0, 0, 0, 0);
+                goto done;
             }
-        /* Only for < 4G file sizes */
-        if (s->dataset.size != 0xFFFFFFFF && rc != s->dataset.size) {
-            usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
-                                 0, 0, 0, 0);
-            goto done;
+            usb_mtp_update_object(parent, s->dataset.filename);
         }
     }
 
@@ -1788,25 +1821,33 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         d->offset = 0;
         d->first = false;
         d->pending = false;
+        d->data_offset = 0;
+        d->write_status = WRITE_START;
     }
 
     if (d->pending) {
-        usb_mtp_realloc(d, MTP_WRITE_BUF_SZ);
-        d->length += MTP_WRITE_BUF_SZ;
+        memset(d->data, 0, d->length);
+        if (d->length != MTP_WRITE_BUF_SZ) {
+            usb_mtp_realloc(d, MTP_WRITE_BUF_SZ - d->length);
+            d->length += (MTP_WRITE_BUF_SZ - d->length);
+        }
         d->pending = false;
+        d->write_status = WRITE_CONTINUE;
+        d->data_offset = 0;
     }
 
-    if (d->length - d->offset > data_len) {
+    if (d->length - d->data_offset > data_len) {
         dlen = data_len;
     } else {
-        dlen = d->length - d->offset;
+        dlen = d->length - d->data_offset;
     }
 
     switch (d->code) {
     case CMD_SEND_OBJECT_INFO:
-        usb_packet_copy(p, d->data + d->offset, dlen);
+        usb_packet_copy(p, d->data + d->data_offset, dlen);
         d->offset += dlen;
-        if (d->offset == d->length) {
+        d->data_offset += dlen;
+        if (d->data_offset == d->length) {
             /* The operation might have already failed */
             if (!s->result) {
                 usb_mtp_write_metadata(s, dlen);
@@ -1817,19 +1858,26 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         }
         break;
     case CMD_SEND_OBJECT:
-        usb_packet_copy(p, d->data + d->offset, dlen);
+        usb_packet_copy(p, d->data + d->data_offset, dlen);
         d->offset += dlen;
+        d->data_offset += dlen;
         if ((p->iov.size % 64) || !p->iov.size) {
             assert((s->dataset.size == 0xFFFFFFFF) ||
                    (s->dataset.size == d->offset));
 
+            if (d->length == MTP_WRITE_BUF_SZ) {
+                d->write_status = WRITE_END;
+            } else {
+                d->write_status = WRITE_START;
+            }
             usb_mtp_write_data(s);
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;
             return;
         }
-        if (d->offset == d->length) {
+        if (d->data_offset == d->length) {
             d->pending = true;
+            usb_mtp_write_data(s);
         }
         break;
     default:
-- 
2.9.3

[Qemu-devel] [PULL 8/8] usb-mtp: replace the homebrew write with qemu_write_full

[Gerd Hoffmann]

From: Bandan Das <bsd@redhat.com>

qemu_write_full takes care of partial blocking writes,
as in cases of larger file sizes

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20190129131908.27924-4-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2ca25c9da5..f1d20fa1b9 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1632,24 +1632,16 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
 /* Wrapper around write, returns 0 on failure */
 static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
 {
-        uint64_t bytes_left = size, ret;
+        uint64_t ret = 0;
 
         if (lseek(fd, offset, SEEK_SET) < 0) {
             goto done;
         }
 
-        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;
-        }
+        ret = qemu_write_full(fd, buf, size);
 
 done:
-        return size - bytes_left;
+        return ret;
 }
 
 static void usb_mtp_update_object(MTPObject *parent, char *name)
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/8] Usb 20190130 patches

[Peter Maydell]

On Wed, 30 Jan 2019 at 07:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit b4fbe1f65a4769c09e6bf2d79fc84360f840f40e:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190129' into staging (2019-01-29 12:00:19 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20190130-pull-request
>
> for you to fetch changes up to 49f9e8d660d41cbe325d554e742d7100d59a7abf:
>
>   usb-mtp: replace the homebrew write with qemu_write_full (2019-01-30 06:47:52 +0100)
>
> ----------------------------------------------------------------
> usb: xhci: fix iso transfers.
> usb: mtp: break up writes, bugfixes.
> usb: fix lgpl info in headers.
> usb: hid: unique serials.
>
> ----------------------------------------------------------------

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [PULL 0/8] Usb 20190130 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190130073426.11525-1-kraxel@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PULL 0/8] Usb 20190130 patches
Type: series
Message-id: 20190130073426.11525-1-kraxel@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
92aad5cacd usb-mtp: replace the homebrew write with qemu_write_full
51e833a06a usb-mtp: breakup MTP write into smaller chunks
de75b18c3e usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ
7679c26cb4 usb: implement XHCI underrun/overrun events
cff5d20762 usb: XHCI shall not halt isochronous endpoints
0d7e030048 hw/usb: Fix LGPL information in the file headers
40bca2e37e usb: dev-mtp: close fd in usb_mtp_object_readdir()
75b3be5381 usb: assign unique serial numbers to hid devices

=== OUTPUT BEGIN ===
1/8 Checking commit 75b3be5381cc (usb: assign unique serial numbers to hid devices)
2/8 Checking commit 40bca2e37e79 (usb: dev-mtp: close fd in usb_mtp_object_readdir())
3/8 Checking commit 0d7e030048b9 (hw/usb: Fix LGPL information in the file headers)
4/8 Checking commit cff5d20762e2 (usb: XHCI shall not halt isochronous endpoints)
5/8 Checking commit 7679c26cb43a (usb: implement XHCI underrun/overrun events)
ERROR: spaces required around that '-' (ctx:VxV)
#37: FILE: hw/usb/hcd-xhci.c:1960:
+                xhci_event(xhci, &ev, xhci->slots[epctx->slotid-1].intr);
                                                                ^

ERROR: spaces required around that '-' (ctx:VxV)
#46: FILE: hw/usb/hcd-xhci.c:2041:
+    xhci->slots[slotid-1].intr = 0;
                       ^

ERROR: spaces required around that '-' (ctx:VxV)
#65: FILE: hw/usb/hcd-xhci.c:2316:
+        xhci->slots[slotid-1].intr = get_field(islot_ctx[2], TRB_INTR);
                           ^

ERROR: spaces required around that '-' (ctx:VxV)
#66: FILE: hw/usb/hcd-xhci.c:2317:
+        set_field(&slot_ctx[2], xhci->slots[slotid-1].intr, TRB_INTR);
                                                   ^

total: 4 errors, 0 warnings, 48 lines checked

Patch 5/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/8 Checking commit de75b18c3e2c (usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ)
7/8 Checking commit 51e833a06a8b (usb-mtp: breakup MTP write into smaller chunks)
8/8 Checking commit 92aad5cacd28 (usb-mtp: replace the homebrew write with qemu_write_full)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190130073426.11525-1-kraxel@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PULL 0/8] Usb 20190130 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190130073426.11525-1-kraxel@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/tmp/qemu-test/install --python=/usr/bin/python3 --cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple --enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 --enable-guest-agent --with-sdlabi=2.0

ERROR: "x86_64-w64-mingw32-gcc" either does not exist or does not work

# QEMU configure log Sat Feb  2 21:26:32 UTC 2019
# Configured with: '/tmp/qemu-test/src/configure' '--enable-werror' '--target-list=x86_64-softmmu,aarch64-softmmu' '--prefix=/tmp/qemu-test/install' '--python=/usr/bin/python3' '--cross-prefix=x86_64-w64-mingw32-' '--enable-trace-backends=simple' '--enable-gnutls' '--enable-nettle' '--enable-curl' '--enable-vnc' '--enable-bzip2' '--enable-guest-agent' '--with-sdlabi=2.0'
---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 636 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 638 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 640 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 642 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 644 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 646 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 648 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 650 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 652 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 654 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 688 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 690 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 696 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 702 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 708 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 710 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 716 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 722 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 619 724 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied

funcs: do_compiler do_cc compile_object main
lines: 92 122 1820 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
ccache: error: Failed to create temporary file for /var/tmp/ccache/tmp/qemu-conf.stdout: Permission denied
Failed to run 'configure'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 563, in <module>


The full log is available at
http://patchew.org/logs/20190130073426.11525-1-kraxel@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Peter Maydell]

On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Bandan Das <bsd@redhat.com>
>
> For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
> getting the next block of data. The file is kept opened for the
> duration of the operation but the sanity checks on the write operation
> are performed only once when the write operation starts. Additionally,
> we also update the file size in the object metadata once the file has
> completely been written.
>
> Suggested-by: Gerd Hoffman <kraxel@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> Message-id: 20190129131908.27924-3-bsd@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Hi; Coverity has spotted a couple of issues with this patch:


> +static void usb_mtp_update_object(MTPObject *parent, char *name)
> +{
> +    MTPObject *o =
> +        usb_mtp_object_lookup_name(parent, name, strlen(name));
> +
> +    if (o) {
> +        lstat(o->path, &o->stat);

CID 1398651: We don't check the return value of this lstat() for failure.

> +    }
> +}
> +
>  static void usb_mtp_write_data(MTPState *s)
>  {
>      MTPData *d = s->data_out;

[...]

> +    case WRITE_CONTINUE:
> +    case WRITE_END:
> +        rc = write_retry(d->fd, d->data, d->data_offset,
> +                         d->offset - d->data_offset);
> +        if (rc != d->data_offset) {
>              usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>                                   0, 0, 0, 0);
>              goto done;
> +        }
> +        if (d->write_status != WRITE_END) {
> +            return;

CID 1398642: This early-return case in usb_mtp_write_data() returns
from the function without doing any of the cleanup (closing file,
freeing data, etc). Possibly it should be "goto done;" instead ?
The specific thing Coverity complains about is the memory pointed
to by "path".

thanks
-- PMM

Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> From: Bandan Das <bsd@redhat.com>
>>
>> For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
>> getting the next block of data. The file is kept opened for the
>> duration of the operation but the sanity checks on the write operation
>> are performed only once when the write operation starts. Additionally,
>> we also update the file size in the object metadata once the file has
>> completely been written.
>>
>> Suggested-by: Gerd Hoffman <kraxel@redhat.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> Message-id: 20190129131908.27924-3-bsd@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Hi; Coverity has spotted a couple of issues with this patch:
>
>
>> +static void usb_mtp_update_object(MTPObject *parent, char *name)
>> +{
>> +    MTPObject *o =
>> +        usb_mtp_object_lookup_name(parent, name, strlen(name));
>> +
>> +    if (o) {
>> +        lstat(o->path, &o->stat);
>
> CID 1398651: We don't check the return value of this lstat() for failure.
>

Thanks, will post a patch for this.

>> +    }
>> +}
>> +
>>  static void usb_mtp_write_data(MTPState *s)
>>  {
>>      MTPData *d = s->data_out;
>
> [...]
>
>> +    case WRITE_CONTINUE:
>> +    case WRITE_END:
>> +        rc = write_retry(d->fd, d->data, d->data_offset,
>> +                         d->offset - d->data_offset);
>> +        if (rc != d->data_offset) {
>>              usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>>                                   0, 0, 0, 0);
>>              goto done;
>> +        }
>> +        if (d->write_status != WRITE_END) {
>> +            return;
>
> CID 1398642: This early-return case in usb_mtp_write_data() returns
> from the function without doing any of the cleanup (closing file,
> freeing data, etc). Possibly it should be "goto done;" instead ?
> The specific thing Coverity complains about is the memory pointed
> to by "path".
>

I believe this is a false positive, there's still more data incoming
and we have successfully written the data we got this time, so we return
without freeing up any of the structures. I will add a comment here.

Bandan

> thanks
> -- PMM

Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Peter Maydell]

On Fri, 15 Feb 2019 at 18:45, Bandan Das <bsd@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > CID 1398642: This early-return case in usb_mtp_write_data() returns
> > from the function without doing any of the cleanup (closing file,
> > freeing data, etc). Possibly it should be "goto done;" instead ?
> > The specific thing Coverity complains about is the memory pointed
> > to by "path".
> >
>
> I believe this is a false positive, there's still more data incoming
> and we have successfully written the data we got this time, so we return
> without freeing up any of the structures. I will add a comment here.

Looking at the code I think Coverity is correct about the 'path'
string. We allocate this with
            path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
and then use it in a mkdir() and an open() call, and never
save the pointer anywhere. But we only g_free() it in the
exit paths that go through "free:".

One simple fix to this would be to narrow the scope of 'path',
so we deal with it only inside the if():

        if (s->dataset.filename) {
            char *path = g_strdup_printf("%s/%s", parent->path,
s->dataset.filename);
            if (s->dataset.format == FMT_ASSOCIATION) {
                d->fd = mkdir(path, mask);
                g_free(path);
                goto free;
            }
            d->fd = open(path, O_CREAT | O_WRONLY |
                         O_CLOEXEC | O_NOFOLLOW, mask);
            g_free(path);
            if (d->fd == -1) {
                usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                     0, 0, 0, 0);
                goto done;
            }
            [...]

and then drop the g_free(path) from the end of the function.


While I'm looking at this, that call to mkdir() looks bogus:
                d->fd = mkdir(path, mask);
mkdir() does not return a file descriptor, it returns a
0-or-negative status code (which we are not checking),
so storing its result into d->fd looks very weird. If
we ever do try to treat d->fd as an fd later on then we
will end up operating on stdin, which is going to have
very confusing effects.


If the MTPState* is kept around and reused, should the
cleanup code that does close(d->fd) also set d->fd to -1,
so that we know that the fd has been closed and don't
later try to close it twice or otherwise use it ?

thanks
-- PMM

Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 15 Feb 2019 at 18:45, Bandan Das <bsd@redhat.com> wrote:
>>
...
>> I believe this is a false positive, there's still more data incoming
>> and we have successfully written the data we got this time, so we return
>> without freeing up any of the structures. I will add a comment here.
>
> Looking at the code I think Coverity is correct about the 'path'
> string. We allocate this with
>             path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
> and then use it in a mkdir() and an open() call, and never
> save the pointer anywhere. But we only g_free() it in the
> exit paths that go through "free:".
>
Thanks for the catch! Indeed, it's not being saved anywhere. I assumed
without looking it's d->path but we don't need path anywhere else, so I guess
your solution below is fair.

> One simple fix to this would be to narrow the scope of 'path',
> so we deal with it only inside the if():
>
>         if (s->dataset.filename) {
>             char *path = g_strdup_printf("%s/%s", parent->path,
> s->dataset.filename);
>             if (s->dataset.format == FMT_ASSOCIATION) {
>                 d->fd = mkdir(path, mask);
>                 g_free(path);
>                 goto free;
>             }
>             d->fd = open(path, O_CREAT | O_WRONLY |
>                          O_CLOEXEC | O_NOFOLLOW, mask);
>             g_free(path);
>             if (d->fd == -1) {
>                 usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>                                      0, 0, 0, 0);
>                 goto done;
>             }
>             [...]
>
> and then drop the g_free(path) from the end of the function.
>
>
> While I'm looking at this, that call to mkdir() looks bogus:
>                 d->fd = mkdir(path, mask);
> mkdir() does not return a file descriptor, it returns a
> 0-or-negative status code (which we are not checking),
> so storing its result into d->fd looks very weird. If
> we ever do try to treat d->fd as an fd later on then we
> will end up operating on stdin, which is going to have
> very confusing effects.
>
Agreed, this is incorrect and confusing. I will add a test
for mkdir. I will post a patch for this as well as set
d->fd to -1. Thanks for taking a look.

Bandan

>
> If the MTPState* is kept around and reused, should the
> cleanup code that does close(d->fd) also set d->fd to -1,
> so that we know that the fd has been closed and don't
> later try to close it twice or otherwise use it ?
>
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 1/8] usb: assign unique serial numbers to hid devices

[Dr. David Alan Gilbert]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
> Windows guests have trouble dealing with usb devices having identical
> serial numbers.  So, assign unique serial numbers to usb hid devices.
> All other usb devices have this already.
> 
> In the past the fixed serial number has been used to indicate working
> remote setup to linux guests.  Here is a bit of history:
> 
>  * First there was nothing.
>  * Then I added a rule to udev checking for serial == 42.
>    (this is in rhel-6).
>  * Then systemd + udev merged.
>  * Then I changed the rule to check for serial != 1 instead, so we can
>    use any serial but "1" which is the one the old broken devices had
>    (this is in rhel-7).  March 2014 in upstream systemd.
>  * Then all usb power management rules where dropped from systemd (June
>    2015).  Which I figured today (Sept 2018), after wondering that the
>    rules are gone in fedora 28.
> 
> So, three years ago the serial number check was dropped upstream, yet I
> hav't seen a single report about autosuspend issues (or cpu usage for
> usb emulation going up, which is the typical symtom).
> 
> So I figured I can stop worring that changing the serial number will
> break things and just do it.
> 
> And even if it turns out autosuspend is still an issue:  I think
> meanwhile we can really stop worrying about guests running in old qemu
> versions with broken usb suspend (fixed in 0.13 !).  If needed we can
> enable autosuspend unconditionally in guests.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-id: 20190110125108.22834-1-kraxel@redhat.com
> ---
>  hw/core/machine.c |  3 +++
>  hw/usb/dev-hid.c  | 26 +++++++++++++++-----------
>  2 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2629515363..077fbd182a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,9 @@ GlobalProperty hw_compat_3_1[] = {
>      { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" },
>      { "tpm-crb", "ppi", "false" },
>      { "tpm-tis", "ppi", "false" },
> +    { "usb-kbd", "serial", "42" },
> +    { "usb-mouse", "serial", "42" },
> +    { "usb-kbd", "serial", "42" },

Hi Gerd,
  There's a copy-pasteism there that happened when you squashed
it down to the new format; you've got the usb-kbd twice
as opposed to having the usb-tablet in there.

Hmm, now how do we fix that? That means if we fix that
now then the usb 3-1 machine type in 4.1 would be the same as
3.1 but different from 4.0; which I suspect is the right fix
at this time.

Dave

>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 90cd745f06..f9ea3033a1 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -61,10 +61,13 @@ enum {
>      STR_PRODUCT_MOUSE,
>      STR_PRODUCT_TABLET,
>      STR_PRODUCT_KEYBOARD,
> -    STR_SERIALNUMBER,
> +    STR_SERIAL_COMPAT,
>      STR_CONFIG_MOUSE,
>      STR_CONFIG_TABLET,
>      STR_CONFIG_KEYBOARD,
> +    STR_SERIAL_MOUSE,
> +    STR_SERIAL_TABLET,
> +    STR_SERIAL_KEYBOARD,
>  };
>  
>  static const USBDescStrings desc_strings = {
> @@ -72,10 +75,13 @@ static const USBDescStrings desc_strings = {
>      [STR_PRODUCT_MOUSE]    = "QEMU USB Mouse",
>      [STR_PRODUCT_TABLET]   = "QEMU USB Tablet",
>      [STR_PRODUCT_KEYBOARD] = "QEMU USB Keyboard",
> -    [STR_SERIALNUMBER]     = "42", /* == remote wakeup works */
> +    [STR_SERIAL_COMPAT]    = "42",
>      [STR_CONFIG_MOUSE]     = "HID Mouse",
>      [STR_CONFIG_TABLET]    = "HID Tablet",
>      [STR_CONFIG_KEYBOARD]  = "HID Keyboard",
> +    [STR_SERIAL_MOUSE]     = "89126",
> +    [STR_SERIAL_TABLET]    = "28754",
> +    [STR_SERIAL_KEYBOARD]  = "68284",
>  };
>  
>  static const USBDescIface desc_iface_mouse = {
> @@ -375,7 +381,7 @@ static const USBDesc desc_mouse = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_MOUSE,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_MOUSE,
>      },
>      .full = &desc_device_mouse,
>      .str  = desc_strings,
> @@ -389,7 +395,7 @@ static const USBDesc desc_mouse2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_MOUSE,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_MOUSE,
>      },
>      .full = &desc_device_mouse,
>      .high = &desc_device_mouse2,
> @@ -404,7 +410,7 @@ static const USBDesc desc_tablet = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_TABLET,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_TABLET,
>      },
>      .full = &desc_device_tablet,
>      .str  = desc_strings,
> @@ -418,7 +424,7 @@ static const USBDesc desc_tablet2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_TABLET,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_TABLET,
>      },
>      .full = &desc_device_tablet,
>      .high = &desc_device_tablet2,
> @@ -433,7 +439,7 @@ static const USBDesc desc_keyboard = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_KEYBOARD,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_KEYBOARD,
>      },
>      .full = &desc_device_keyboard,
>      .str  = desc_strings,
> @@ -447,7 +453,7 @@ static const USBDesc desc_keyboard2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_KEYBOARD,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_KEYBOARD,
>      },
>      .full = &desc_device_keyboard,
>      .high = &desc_device_keyboard2,
> @@ -718,9 +724,7 @@ static void usb_hid_initfn(USBDevice *dev, int kind,
>          return;
>      }
>  
> -    if (dev->serial) {
> -        usb_desc_set_string(dev, STR_SERIALNUMBER, dev->serial);
> -    }
> +    usb_desc_create_serial(dev);
>      usb_desc_init(dev);
>      us->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
>      hid_init(&us->hid, kind, usb_hid_changed);
> -- 
> 2.9.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK