Patchwork usb/vmstate: add parent dev path

login
register
mail settings
Submitter Gerd Hoffmann
Date March 22, 2012, 12:07 p.m.
Message ID <1332418064-23091-1-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/148230/
State New
Headers show

Comments

Gerd Hoffmann - March 22, 2012, 12:07 p.m.
... to make vmstate id string truely unique with multiple host
controllers, i.e. move from "1/usb-ptr" to "0000:00:01.3/1/usb-ptr"
(usb tabled connected to piix3 uhci).

This obviously breaks migration.  To handle this the usb bus
property "full-path" is added.  When setting this to false old
behavior is maintained.  This way current qemu will be compatible
with old versions when started using '-M pc-$oldversion'.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc_piix.c |   28 ++++++++++++++++++++++++++++
 hw/usb.h     |    5 +++++
 hw/usb/bus.c |   18 +++++++++++++++++-
 3 files changed, 50 insertions(+), 1 deletions(-)
Paolo Bonzini - March 22, 2012, 12:17 p.m.
Il 22/03/2012 13:07, Gerd Hoffmann ha scritto:
> @@ -460,7 +462,21 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
>  static char *usb_get_dev_path(DeviceState *qdev)
>  {
>      USBDevice *dev = USB_DEVICE(qdev);
> -    return g_strdup(dev->port->path);
> +    DeviceState *hcd = qdev->parent_bus->parent;
> +    char *id = NULL;
> +
> +    if ((dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) &&
> +        hcd && hcd->parent_bus && hcd->parent_bus->info->get_dev_path) {
> +        id = hcd->parent_bus->info->get_dev_path(hcd);
> +    }
> +    if (id) {
> +        int len = strlen(id)+strlen(dev->port->path)+2;
> +        char *ret = g_malloc(len);
> +        snprintf(ret, len, "%s/%s", id, dev->port->path);

You can use g_strdup_printf here, also this is leaking id (I have to fix
it for SCSI too).

> +        return ret;
> +    } else {
> +        return g_strdup(dev->port->path);
> +    }
>  }
>  
>  static char *usb_get_fw_dev_path(DeviceState *qdev)

Paolo
Andreas Färber - March 22, 2012, 2:01 p.m.
Am 22.03.2012 13:07, schrieb Gerd Hoffmann:
> ... to make vmstate id string truely unique with multiple host
> controllers, i.e. move from "1/usb-ptr" to "0000:00:01.3/1/usb-ptr"
> (usb tabled connected to piix3 uhci).
> 
> This obviously breaks migration.  To handle this the usb bus
> property "full-path" is added.  When setting this to false old
> behavior is maintained.  This way current qemu will be compatible
> with old versions when started using '-M pc-$oldversion'.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pc_piix.c |   28 ++++++++++++++++++++++++++++
>  hw/usb.h     |    5 +++++
>  hw/usb/bus.c |   18 +++++++++++++++++-
>  3 files changed, 50 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3f99f9a..8cb7d5f 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
>              .driver   = "isa-fdc",
>              .property = "check_media_rate",
>              .value    = "off",
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",

This touches on our "favorite" bit/bool topic again. While I agree that
"no" makes sense for a property of that name, the current code still
expects "on" and "off". In particular, "yes" would not work as expected.

We should either merge support for yes/no or use "off" here.
I had a patch for the former but didn't resubmit after the QOM merge
since I thought we would want to move this from qdev to Object first.

Andreas

>          },
>          { /* end of list */ }
>      },
> @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
>              .driver   = "isa-fdc",
>              .property = "check_media_rate",
>              .value    = "off",
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      },
> @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      },
> @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      },
> @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      }
> @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      }
> @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        },{
> +            .driver   = "USB",
> +            .property = "full-path",
> +            .value    = "no",
>          },
>          { /* end of list */ }
>      },

> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index d3f8358..f1e567b 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -19,6 +19,8 @@ static struct BusInfo usb_bus_info = {
>      .get_fw_dev_path = usb_get_fw_dev_path,
>      .props      = (Property[]) {
>          DEFINE_PROP_STRING("port", USBDevice, port_path),
> +        DEFINE_PROP_BIT("full-path", USBDevice, flags,
> +                        USB_DEV_FLAG_FULL_PATH, true),
>          DEFINE_PROP_END_OF_LIST()
>      },
>  };
Gerd Hoffmann - March 22, 2012, 2:37 p.m.
Hi,

>> +            .driver   = "USB",
>> +            .property = "full-path",
>> +            .value    = "no",
> 
> This touches on our "favorite" bit/bool topic again. While I agree that
> "no" makes sense for a property of that name, the current code still
> expects "on" and "off". In particular, "yes" would not work as expected.

"no" *does* work as expected though, at least it survived my tests,
thats why I didn't notice.

I can s/no/off/ for consistency, no problem.  Merging your yes/no patch
is fine with me too.

cheers,
  Gerd
Andreas Färber - March 22, 2012, 3:12 p.m.
Am 22.03.2012 15:37, schrieb Gerd Hoffmann:
>>> +            .driver   = "USB",
>>> +            .property = "full-path",
>>> +            .value    = "no",
>>
>> This touches on our "favorite" bit/bool topic again. While I agree that
>> "no" makes sense for a property of that name, the current code still
>> expects "on" and "off". In particular, "yes" would not work as expected.
> 
> "no" *does* work as expected though, at least it survived my tests,
> thats why I didn't notice.
> 
> I can s/no/off/ for consistency, no problem.  Merging your yes/no patch
> is fine with me too.

I stand corrected: Michael Roth's conversion of qdev properties to use
visitors implicitly changed the parsing logic to cover yes/no and
true/false as well. That's great news!

So no change of .value is needed here. The only thing to note is that
print_bit() will output the value as on/off.

Sorry for the confusion,

Andreas
Michael Roth - March 28, 2012, 10:08 p.m.
On Thu, Mar 22, 2012 at 04:12:12PM +0100, Andreas Färber wrote:
> Am 22.03.2012 15:37, schrieb Gerd Hoffmann:
> >>> +            .driver   = "USB",
> >>> +            .property = "full-path",
> >>> +            .value    = "no",
> >>
> >> This touches on our "favorite" bit/bool topic again. While I agree that
> >> "no" makes sense for a property of that name, the current code still
> >> expects "on" and "off". In particular, "yes" would not work as expected.
> > 
> > "no" *does* work as expected though, at least it survived my tests,
> > thats why I didn't notice.
> > 
> > I can s/no/off/ for consistency, no problem.  Merging your yes/no patch
> > is fine with me too.
> 
> I stand corrected: Michael Roth's conversion of qdev properties to use
> visitors implicitly changed the parsing logic to cover yes/no and
> true/false as well. That's great news!

I believe Paolo did that conversion, but agreed :)

> 
> So no change of .value is needed here. The only thing to note is that
> print_bit() will output the value as on/off.
> 
> Sorry for the confusion,
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Patch

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3f99f9a..8cb7d5f 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -386,6 +386,10 @@  static QEMUMachine pc_machine_v1_0 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     },
@@ -405,6 +409,10 @@  static QEMUMachine pc_machine_v0_15 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     },
@@ -449,6 +457,10 @@  static QEMUMachine pc_machine_v0_14 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     },
@@ -505,6 +517,10 @@  static QEMUMachine pc_machine_v0_13 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     },
@@ -565,6 +581,10 @@  static QEMUMachine pc_machine_v0_12 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     }
@@ -633,6 +653,10 @@  static QEMUMachine pc_machine_v0_11 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     }
@@ -713,6 +737,10 @@  static QEMUMachine pc_machine_v0_10 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        },{
+            .driver   = "USB",
+            .property = "full-path",
+            .value    = "no",
         },
         { /* end of list */ }
     },
diff --git a/hw/usb.h b/hw/usb.h
index e95085f..ae7ccda 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -182,12 +182,17 @@  struct USBEndpoint {
     QTAILQ_HEAD(, USBPacket) queue;
 };
 
+enum USBDeviceFlags {
+    USB_DEV_FLAG_FULL_PATH,
+};
+
 /* definition of a USB device */
 struct USBDevice {
     DeviceState qdev;
     USBPort *port;
     char *port_path;
     void *opaque;
+    uint32_t flags;
 
     /* Actual connected speed */
     int speed;
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index d3f8358..f1e567b 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -19,6 +19,8 @@  static struct BusInfo usb_bus_info = {
     .get_fw_dev_path = usb_get_fw_dev_path,
     .props      = (Property[]) {
         DEFINE_PROP_STRING("port", USBDevice, port_path),
+        DEFINE_PROP_BIT("full-path", USBDevice, flags,
+                        USB_DEV_FLAG_FULL_PATH, true),
         DEFINE_PROP_END_OF_LIST()
     },
 };
@@ -460,7 +462,21 @@  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 static char *usb_get_dev_path(DeviceState *qdev)
 {
     USBDevice *dev = USB_DEVICE(qdev);
-    return g_strdup(dev->port->path);
+    DeviceState *hcd = qdev->parent_bus->parent;
+    char *id = NULL;
+
+    if ((dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) &&
+        hcd && hcd->parent_bus && hcd->parent_bus->info->get_dev_path) {
+        id = hcd->parent_bus->info->get_dev_path(hcd);
+    }
+    if (id) {
+        int len = strlen(id)+strlen(dev->port->path)+2;
+        char *ret = g_malloc(len);
+        snprintf(ret, len, "%s/%s", id, dev->port->path);
+        return ret;
+    } else {
+        return g_strdup(dev->port->path);
+    }
 }
 
 static char *usb_get_fw_dev_path(DeviceState *qdev)