| Message ID | 1389121639-17657-1-git-send-email-kroosec@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hani Benhabiles <kroosec@gmail.com> writes: > This would allow a user to be able to refer to the device when using commands > like device_del. > > Signed-off-by: Hani Benhabiles <kroosec@gmail.com> No. Device IDs belong to the user. Any IDs the system picks automatically can collide with the user's IDs. Management applications assume that they can pick any ID they want. Your patch can introduce ID collisions, and thus make existing configurations fail. If I remember correctly, a few legacy convenience options pick IDs for historical reasons. If you use them, you need to be aware of the IDs they pick. Management applications shouldn't use them. We've discussed this a couple of times already, by the way.
On Wed, Jan 08, 2014 at 08:36:06AM +0100, Markus Armbruster wrote: > Hani Benhabiles <kroosec@gmail.com> writes: > > > This would allow a user to be able to refer to the device when using commands > > like device_del. > > > > Signed-off-by: Hani Benhabiles <kroosec@gmail.com> > > No. > > Device IDs belong to the user. Any IDs the system picks automatically > can collide with the user's IDs. > > Management applications assume that they can pick any ID they want. > Your patch can introduce ID collisions, and thus make existing > configurations fail. > How can it lead to ID collisions ? For this reason, the loop in assign_device_name() specifically check that the ID doesn't exist already and uses the next value if it does. How would something like: (qemu) device_add virtio-net-pci ==> ID: virtio-net-pci.0 Be more problematic than: (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 > If I remember correctly, a few legacy convenience options pick IDs for > historical reasons. If you use them, you need to be aware of the IDs > they pick. Management applications shouldn't use them. > > We've discussed this a couple of times already, by the way.
Il 08/01/2014 18:17, Hani Benhabiles ha scritto: > For this reason, the loop in assign_device_name() specifically check that the ID > doesn't exist already and uses the next value if it does. > > How would something like: > (qemu) device_add virtio-net-pci > ==> ID: virtio-net-pci.0 > > Be more problematic than: > (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 (qemu) device_add virtio-net-pci (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 works without your patches, fails with them (IIUC). Paolo
On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote: > Il 08/01/2014 18:17, Hani Benhabiles ha scritto: > > For this reason, the loop in assign_device_name() specifically check that the ID > > doesn't exist already and uses the next value if it does. > > > > How would something like: > > (qemu) device_add virtio-net-pci > > ==> ID: virtio-net-pci.0 > > > > Be more problematic than: > > (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 > > (qemu) device_add virtio-net-pci > (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 > > works without your patches, fails with them (IIUC). It would fail with a clear and descriptive message (Duplicate ID) which is no different than when a user uses tries to do so manually. Without the patch, how does a user unplug a device for which he didn't specify an ID (eg. forgotten, added quickly...) It is even more confusing with other commands like info networks. (qemu) device_add virtio-net-pci (qemu) device_add virtio-net-pci,id=foo (qemu) info network virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57 (qemu) device_del foo (qemu) device_del virtio-net-pci.0 Device 'virtio-net-pci.0' not found If the naming scheme is an issue, could something like starting it with one or two underscores better suited ? Cheers, Hani.
Il 09/01/2014 19:18, Hani Benhabiles ha scritto: > On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote: >> Il 08/01/2014 18:17, Hani Benhabiles ha scritto: >>> For this reason, the loop in assign_device_name() specifically check that the ID >>> doesn't exist already and uses the next value if it does. >>> >>> How would something like: >>> (qemu) device_add virtio-net-pci >>> ==> ID: virtio-net-pci.0 >>> >>> Be more problematic than: >>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 >> >> (qemu) device_add virtio-net-pci >> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 >> >> works without your patches, fails with them (IIUC). > > It would fail with a clear and descriptive message (Duplicate ID) which is no > different than when a user uses tries to do so manually. It would still be a regression. > Without the patch, how does a user unplug a device for which he didn't specify > an ID (eg. forgotten, added quickly...) He doesn't. :) > It is even more confusing with other commands like info networks. > > (qemu) device_add virtio-net-pci > (qemu) device_add virtio-net-pci,id=foo > (qemu) info network > virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 > foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57 > (qemu) device_del foo > (qemu) device_del virtio-net-pci.0 > Device 'virtio-net-pci.0' not found This could be considered a bug in "info network" too. You might use a different template such as "unnamed virtio-net-pci #1" in net/net.c's assign_name. Paolo > If the naming scheme is an issue, could something like starting it with one or > two underscores better suited ? > > Cheers, > > Hani. > >
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 09/01/2014 19:18, Hani Benhabiles ha scritto: >> On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote: >>> Il 08/01/2014 18:17, Hani Benhabiles ha scritto: >>>> For this reason, the loop in assign_device_name() specifically >>>> check that the ID >>>> doesn't exist already and uses the next value if it does. >>>> >>>> How would something like: >>>> (qemu) device_add virtio-net-pci >>>> ==> ID: virtio-net-pci.0 >>>> >>>> Be more problematic than: >>>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 >>> >>> (qemu) device_add virtio-net-pci >>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0 >>> >>> works without your patches, fails with them (IIUC). >> >> It would fail with a clear and descriptive message (Duplicate ID) which is no >> different than when a user uses tries to do so manually. > > It would still be a regression. > >> Without the patch, how does a user unplug a device for which he didn't specify >> an ID (eg. forgotten, added quickly...) > > He doesn't. :) > >> It is even more confusing with other commands like info networks. >> >> (qemu) device_add virtio-net-pci >> (qemu) device_add virtio-net-pci,id=foo >> (qemu) info network >> virtio-net-pci.0: >> index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56 >> foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57 >> (qemu) device_del foo >> (qemu) device_del virtio-net-pci.0 >> Device 'virtio-net-pci.0' not found > > This could be considered a bug in "info network" too. You might use a > different template such as "unnamed virtio-net-pci #1" in net/net.c's > assign_name. > > Paolo > >> If the naming scheme is an issue, could something like starting it with one or >> two underscores better suited ? In the design of qdev, the path in the qtree is the address that always works, and IDs are abbreviations the user can define for his convenience. Unfortunately, we botched qtree paths so badly, they're effectively unusable. We consequently never made them available as command arguments. They only occur as values of the bus property, if I remember correctly. One possible recovery plan is to give up on paths, and auto-generate IDs. Another one is to ditch the unusable legacy qtree paths, and adopt the "real" paths instead, namely the QOM paths. I like this plan better. Commands taking a device ID could be extended to take a path in the QOM graph instead of an ID. In the human monitor, it could perhaps work like this: IDs consist of letters, digits, '-', '.', '_', starting with a letter (see id_wellformed()) If the argument doesn't contain '/', interpret it as ID. Else, if it starts with '/', interpret it as QOM path anchored at "the root" (which needs to be defined). Else, split it at the first '/', and interpret the prefix as ID, and the suffix as as QOM path anchored at whatever has that ID. Requires means to inspect the QOM graph to be usable.
Am 10.01.2014 10:09, schrieb Markus Armbruster: > Commands taking a device ID could be extended to take a path in the QOM > graph instead of an ID. > > In the human monitor, it could perhaps work like this: > > IDs consist of letters, digits, '-', '.', '_', starting with a > letter (see id_wellformed()) > > If the argument doesn't contain '/', interpret it as ID. > Else, if it starts with '/', interpret it as QOM path anchored at > "the root" (which needs to be defined). > Else, split it at the first '/', and interpret the prefix as ID, and > the suffix as as QOM path anchored at whatever has that ID. > > Requires means to inspect the QOM graph to be usable. qom-list is available for that purpose today. Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 10.01.2014 10:09, schrieb Markus Armbruster: >> Commands taking a device ID could be extended to take a path in the QOM >> graph instead of an ID. >> >> In the human monitor, it could perhaps work like this: >> >> IDs consist of letters, digits, '-', '.', '_', starting with a >> letter (see id_wellformed()) >> >> If the argument doesn't contain '/', interpret it as ID. >> Else, if it starts with '/', interpret it as QOM path anchored at >> "the root" (which needs to be defined). >> Else, split it at the first '/', and interpret the prefix as ID, and >> the suffix as as QOM path anchored at whatever has that ID. >> >> Requires means to inspect the QOM graph to be usable. > > qom-list is available for that purpose today. We'd need something in the human monitor, too.
diff --git a/qdev-monitor.c b/qdev-monitor.c index dc37a43..64b2b22 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -451,11 +451,54 @@ static BusState *qbus_find(const char *path) } } +static int qbus_device_count(const BusState *bus, const char *typename) +{ + BusChild *kid; + int count = 0; + + QTAILQ_FOREACH(kid, &bus->children, sibling) { + DeviceState *dev = kid->child; + BusState *dev_child; + + if (!strcmp(typename, object_get_typename(OBJECT(dev)))) { + count++; + } + + QLIST_FOREACH(dev_child, &dev->child_bus, sibling) { + count += qbus_device_count(dev_child, typename); + } + } + + return count; +} + +static gchar *assign_device_name(const char *typename, const DeviceState *dev) +{ + BusState *bus; + gchar *new_id; + int count = 0; + + bus = sysbus_get_default(); + count += qbus_device_count(bus, typename); + + while (1) { + new_id = g_strdup_printf("%s.%d", typename, count - 1); + /* To not use an existing ID, if the user previously specified it. */ + if (!qdev_find_recursive(sysbus_get_default(), new_id)) { + break; + } + count++; + g_free(new_id); + } + + return new_id; +} + DeviceState *qdev_device_add(QemuOpts *opts) { ObjectClass *oc; DeviceClass *dc; - const char *driver, *path, *id; + const char *driver, *path; DeviceState *dev; BusState *bus = NULL; Error *err = NULL; @@ -522,25 +565,18 @@ DeviceState *qdev_device_add(QemuOpts *opts) qdev_set_parent_bus(dev, bus); } - id = qemu_opts_id(opts); - if (id) { - dev->id = id; + dev->id = qemu_opts_id(opts); + if (!dev->id) { + qemu_opts_set_id(opts, assign_device_name(driver, dev)); + dev->id = qemu_opts_id(opts); } if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) { object_unparent(OBJECT(dev)); object_unref(OBJECT(dev)); return NULL; } - if (dev->id) { - object_property_add_child(qdev_get_peripheral(), dev->id, - OBJECT(dev), NULL); - } else { - static int anon_count; - gchar *name = g_strdup_printf("device[%d]", anon_count++); - object_property_add_child(qdev_get_peripheral_anon(), name, - OBJECT(dev), NULL); - g_free(name); - } + object_property_add_child(qdev_get_peripheral(), dev->id, OBJECT(dev), + NULL); object_property_set_bool(OBJECT(dev), true, "realized", &err); if (err != NULL) { qerror_report_err(err);
This would allow a user to be able to refer to the device when using commands like device_del. Signed-off-by: Hani Benhabiles <kroosec@gmail.com> --- qdev-monitor.c | 64 +++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 14 deletions(-)