| Message ID | 1332418064-23091-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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() > }, > };
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
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
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 >
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)
... 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(-)