| Message ID | m3d3wcrwg7.fsf_-_@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
On 05/31/10 16:13, Markus Armbruster wrote: > We need Device IDs to be unique and not contain '/' so device tree > nodes can always be unambigously referenced by tree path. > > We already have some protection against duplicate IDs, but it got > holes: > > * We don't assign IDs to default devices. > > * -device and device_add use the ID of a qemu_device_opts. Which > rejects duplicate IDs. > > * pci_add nic -net use either the ID or option "name" of > qemu_net_opts. And there's our hole. Reproducible with "-net user > -net nic,id=foo -device lsi,id=foo". > > Also require IDs to start with a letter to provide for possible future > extensions. > Acked-by: Gerd Hoffmann <kraxel@redhat.com> cheers, Gerd
On Mon, 31 May 2010 16:13:12 +0200 Markus Armbruster <armbru@redhat.com> wrote: > We need Device IDs to be unique and not contain '/' so device tree > nodes can always be unambigously referenced by tree path. > > We already have some protection against duplicate IDs, but it got > holes: > > * We don't assign IDs to default devices. > > * -device and device_add use the ID of a qemu_device_opts. Which > rejects duplicate IDs. > > * pci_add nic -net use either the ID or option "name" of > qemu_net_opts. And there's our hole. Reproducible with "-net user > -net nic,id=foo -device lsi,id=foo". Two bugs that might not be related to this thread: * "id" member is not mandatory for the device_add command: { "execute": "device_add", "arguments": { "driver": "e1000" } } {"return": {}} * "id" member remains in use when the netdev_add command fails: { "execute": "netdev_add", "arguments": { "id": "foobar" } } {"error": {"class": "MissingParameter", "desc": "Parameter 'type' is missing", "data": {"name": "type"}}} { "execute": "netdev_add", "arguments": { "type": "user", "id": "foobar" } } {"error": {"class": "DuplicateId", "desc": "Duplicate ID 'foobar' for netdev", "data": {"object": "netdev", "id": "foobar"}}}
Luiz Capitulino wrote: > Two bugs that might not be related to this thread: > > * "id" member is not mandatory for the device_add command: > > { "execute": "device_add", "arguments": { "driver": "e1000" } } > {"return": {}} Once we enable qtree paths for device_del, this is no longer an issue, devices will remain addressable. Jan
On Tue, 01 Jun 2010 15:09:34 +0200 Jan Kiszka <jan.kiszka@siemens.com> wrote: > Luiz Capitulino wrote: > > Two bugs that might not be related to this thread: > > > > * "id" member is not mandatory for the device_add command: > > > > { "execute": "device_add", "arguments": { "driver": "e1000" } } > > {"return": {}} > > Once we enable qtree paths for device_del, this is no longer an issue, > devices will remain addressable. Main point is whether "id" is required or not, I think it should be.
Luiz Capitulino wrote: > On Tue, 01 Jun 2010 15:09:34 +0200 > Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> Luiz Capitulino wrote: >>> Two bugs that might not be related to this thread: >>> >>> * "id" member is not mandatory for the device_add command: >>> >>> { "execute": "device_add", "arguments": { "driver": "e1000" } } >>> {"return": {}} >> Once we enable qtree paths for device_del, this is no longer an issue, >> devices will remain addressable. > > Main point is whether "id" is required or not, I think it should be. And I think it might be recommended but should become mandatory (specifically not for HMP). Jan
On 06/01/2010 04:19 PM, Jan Kiszka wrote: > >> Main point is whether "id" is required or not, I think it should be. >> > And I think it might be recommended but should become mandatory > (specifically not for HMP). > I agree. Making id a mandatory argument will just result in clients being forced to manufacture unnecessary IDs, which is annoying.
On Tue, 01 Jun 2010 15:19:59 +0200 Jan Kiszka <jan.kiszka@siemens.com> wrote: > Luiz Capitulino wrote: > > On Tue, 01 Jun 2010 15:09:34 +0200 > > Jan Kiszka <jan.kiszka@siemens.com> wrote: > > > >> Luiz Capitulino wrote: > >>> Two bugs that might not be related to this thread: > >>> > >>> * "id" member is not mandatory for the device_add command: > >>> > >>> { "execute": "device_add", "arguments": { "driver": "e1000" } } > >>> {"return": {}} > >> Once we enable qtree paths for device_del, this is no longer an issue, > >> devices will remain addressable. > > > > Main point is whether "id" is required or not, I think it should be. > > And I think it might be recommended but should become mandatory > (specifically not for HMP). Maybe you meant 'should not'? Anyway, if we keep it optional for device_add we should also change the others _add commands to have the same behavior.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Mon, 31 May 2010 16:13:12 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> We need Device IDs to be unique and not contain '/' so device tree >> nodes can always be unambigously referenced by tree path. >> >> We already have some protection against duplicate IDs, but it got >> holes: >> >> * We don't assign IDs to default devices. >> >> * -device and device_add use the ID of a qemu_device_opts. Which >> rejects duplicate IDs. >> >> * pci_add nic -net use either the ID or option "name" of >> qemu_net_opts. And there's our hole. Reproducible with "-net user >> -net nic,id=foo -device lsi,id=foo". > > Two bugs that might not be related to this thread: > > * "id" member is not mandatory for the device_add command: > > { "execute": "device_add", "arguments": { "driver": "e1000" } } > {"return": {}} Works as designed. > * "id" member remains in use when the netdev_add command fails: > > { "execute": "netdev_add", "arguments": { "id": "foobar" } } > {"error": {"class": "MissingParameter", "desc": "Parameter 'type' is missing", "data": {"name": "type"}}} > > { "execute": "netdev_add", "arguments": { "type": "user", "id": "foobar" } } > {"error": {"class": "DuplicateId", "desc": "Duplicate ID 'foobar' for netdev", "data": {"object": "netdev", "id": "foobar"}}} Sounds like a bug.
On Tue, 01 Jun 2010 16:44:24 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Mon, 31 May 2010 16:13:12 +0200 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> We need Device IDs to be unique and not contain '/' so device tree > >> nodes can always be unambigously referenced by tree path. > >> > >> We already have some protection against duplicate IDs, but it got > >> holes: > >> > >> * We don't assign IDs to default devices. > >> > >> * -device and device_add use the ID of a qemu_device_opts. Which > >> rejects duplicate IDs. > >> > >> * pci_add nic -net use either the ID or option "name" of > >> qemu_net_opts. And there's our hole. Reproducible with "-net user > >> -net nic,id=foo -device lsi,id=foo". > > > > Two bugs that might not be related to this thread: > > > > * "id" member is not mandatory for the device_add command: > > > > { "execute": "device_add", "arguments": { "driver": "e1000" } } > > {"return": {}} > > Works as designed. What about netdev_add? { "execute": "netdev_add", "arguments": { "type": "user" } } {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}}
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Tue, 01 Jun 2010 16:44:24 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > On Mon, 31 May 2010 16:13:12 +0200 >> > Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> We need Device IDs to be unique and not contain '/' so device tree >> >> nodes can always be unambigously referenced by tree path. >> >> >> >> We already have some protection against duplicate IDs, but it got >> >> holes: >> >> >> >> * We don't assign IDs to default devices. >> >> >> >> * -device and device_add use the ID of a qemu_device_opts. Which >> >> rejects duplicate IDs. >> >> >> >> * pci_add nic -net use either the ID or option "name" of >> >> qemu_net_opts. And there's our hole. Reproducible with "-net user >> >> -net nic,id=foo -device lsi,id=foo". >> > >> > Two bugs that might not be related to this thread: >> > >> > * "id" member is not mandatory for the device_add command: >> > >> > { "execute": "device_add", "arguments": { "driver": "e1000" } } >> > {"return": {}} >> >> Works as designed. > > What about netdev_add? > > { "execute": "netdev_add", "arguments": { "type": "user" } } > {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}} The only way to put a netdev to use is connecting it to a NIC with -device DRIVER,netdev=ID,... And that requires an ID.
On 06/01/2010 01:35 PM, Markus Armbruster wrote: > Luiz Capitulino<lcapitulino@redhat.com> writes: > > >> On Tue, 01 Jun 2010 16:44:24 +0200 >> Markus Armbruster<armbru@redhat.com> wrote: >> >> >>> Luiz Capitulino<lcapitulino@redhat.com> writes: >>> >>> >>>> On Mon, 31 May 2010 16:13:12 +0200 >>>> Markus Armbruster<armbru@redhat.com> wrote: >>>> >>>> >>>>> We need Device IDs to be unique and not contain '/' so device tree >>>>> nodes can always be unambigously referenced by tree path. >>>>> >>>>> We already have some protection against duplicate IDs, but it got >>>>> holes: >>>>> >>>>> * We don't assign IDs to default devices. >>>>> >>>>> * -device and device_add use the ID of a qemu_device_opts. Which >>>>> rejects duplicate IDs. >>>>> >>>>> * pci_add nic -net use either the ID or option "name" of >>>>> qemu_net_opts. And there's our hole. Reproducible with "-net user >>>>> -net nic,id=foo -device lsi,id=foo". >>>>> >>>> Two bugs that might not be related to this thread: >>>> >>>> * "id" member is not mandatory for the device_add command: >>>> >>>> { "execute": "device_add", "arguments": { "driver": "e1000" } } >>>> {"return": {}} >>>> >>> Works as designed. >>> >> What about netdev_add? >> >> { "execute": "netdev_add", "arguments": { "type": "user" } } >> {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}} >> > The only way to put a netdev to use is connecting it to a NIC with > -device DRIVER,netdev=ID,... And that requires an ID. > To be honest, I think we should universally require an id parameter or we should auto-assign one and return it during creation. Implicit/Null ids with QemuOpts are really difficult to work with and it certainly makes device addressing a lot easier. Regards, Anthony Liguori
Anthony Liguori wrote: > On 06/01/2010 01:35 PM, Markus Armbruster wrote: >> Luiz Capitulino<lcapitulino@redhat.com> writes: >> >> >>> On Tue, 01 Jun 2010 16:44:24 +0200 >>> Markus Armbruster<armbru@redhat.com> wrote: >>> >>> >>>> Luiz Capitulino<lcapitulino@redhat.com> writes: >>>> >>>> >>>>> On Mon, 31 May 2010 16:13:12 +0200 >>>>> Markus Armbruster<armbru@redhat.com> wrote: >>>>> >>>>> >>>>>> We need Device IDs to be unique and not contain '/' so device tree >>>>>> nodes can always be unambigously referenced by tree path. >>>>>> >>>>>> We already have some protection against duplicate IDs, but it got >>>>>> holes: >>>>>> >>>>>> * We don't assign IDs to default devices. >>>>>> >>>>>> * -device and device_add use the ID of a qemu_device_opts. Which >>>>>> rejects duplicate IDs. >>>>>> >>>>>> * pci_add nic -net use either the ID or option "name" of >>>>>> qemu_net_opts. And there's our hole. Reproducible with "-net >>>>>> user >>>>>> -net nic,id=foo -device lsi,id=foo". >>>>>> >>>>> Two bugs that might not be related to this thread: >>>>> >>>>> * "id" member is not mandatory for the device_add command: >>>>> >>>>> { "execute": "device_add", "arguments": { "driver": "e1000" } } >>>>> {"return": {}} >>>>> >>>> Works as designed. >>>> >>> What about netdev_add? >>> >>> { "execute": "netdev_add", "arguments": { "type": "user" } } >>> {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is >>> missing", "data": {"name": "id"}}} >>> >> The only way to put a netdev to use is connecting it to a NIC with >> -device DRIVER,netdev=ID,... And that requires an ID. >> > > To be honest, I think we should universally require an id parameter or > we should auto-assign one and return it during creation. > > Implicit/Null ids with QemuOpts are really difficult to work with and it > certainly makes device addressing a lot easier. I really see no need for this additional burden. We always have unique qtree paths for devices. Jan
> Also require IDs to start with a letter to provide for possible future > extensions. I'd go further than that, and require that user specified IDs match [A-Za-z][A-Za-z0-9_-]* Paul
Paul Brook <paul@codesourcery.com> writes: >> Also require IDs to start with a letter to provide for possible future >> extensions. > > I'd go further than that, and require that user specified IDs match > [A-Za-z][A-Za-z0-9_-]* I talked with Dan (cc'ed) to make sure we don't trample on existing libvirt usage without need. What about [A-Za-z][A-Za-z0-9_-:.]* i.e. your regexp plus ':' and '.' in the second bracket?
> Paul Brook <paul@codesourcery.com> writes: > >> Also require IDs to start with a letter to provide for possible future > >> extensions. > > > > I'd go further than that, and require that user specified IDs match > > [A-Za-z][A-Za-z0-9_-]* > > I talked with Dan (cc'ed) to make sure we don't trample on existing > libvirt usage without need. What about > > [A-Za-z][A-Za-z0-9_-:.]* > > i.e. your regexp plus ':' and '.' in the second bracket? I was deliberately avoiding those as they're often used as separators - we already use ':' in other contexts so there's potential ambiguity and parsing/quoting issues. I'm not aware of any current issues with '.'. Paul P.S. Your regexp doesn't do what you think it does, but I know what you mean.
Paul Brook <paul@codesourcery.com> writes: >> Paul Brook <paul@codesourcery.com> writes: >> >> Also require IDs to start with a letter to provide for possible future >> >> extensions. >> > >> > I'd go further than that, and require that user specified IDs match >> > [A-Za-z][A-Za-z0-9_-]* >> >> I talked with Dan (cc'ed) to make sure we don't trample on existing >> libvirt usage without need. What about >> >> [A-Za-z][A-Za-z0-9_-:.]* >> >> i.e. your regexp plus ':' and '.' in the second bracket? > > I was deliberately avoiding those as they're often used as separators - we > already use ':' in other contexts so there's potential ambiguity and > parsing/quoting issues. I'm not aware of any current issues with '.'. I checked with Dan, and we can outlaw ':'. I just posted a new patch that covers all qemu-option IDs, not just qdev. [...]
diff --git a/hw/qdev.c b/hw/qdev.c index af17486..877cdf4 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -40,6 +40,7 @@ DeviceInfo *device_info_list; static BusState *qbus_find_recursive(BusState *bus, const char *name, const BusInfo *info); static BusState *qbus_find(const char *path); +static DeviceState *qdev_find_recursive(BusState *bus, const char *id); /* Register a new device type. */ void qdev_register(DeviceInfo *info) @@ -242,6 +243,16 @@ DeviceState *qdev_device_add(QemuOpts *opts) qdev = qdev_create_from_info(bus, info); id = qemu_opts_id(opts); if (id) { + if (!qemu_isalpha(id[0]) || strchr(id, '/')) { + qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", + "an identifier starting with a letter, " + "and not containing '/'"); + return NULL; + } + if (qdev_find_recursive(main_system_bus, id)) { + qerror_report(QERR_DUPLICATE_ID, id, "device"); + return NULL; + } qdev->id = id; } if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
We need Device IDs to be unique and not contain '/' so device tree nodes can always be unambigously referenced by tree path. We already have some protection against duplicate IDs, but it got holes: * We don't assign IDs to default devices. * -device and device_add use the ID of a qemu_device_opts. Which rejects duplicate IDs. * pci_add nic -net use either the ID or option "name" of qemu_net_opts. And there's our hole. Reproducible with "-net user -net nic,id=foo -device lsi,id=foo". Also require IDs to start with a letter to provide for possible future extensions. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- Note: If requiring IDs to start with a letter should turn out to be controversial, I'm happy to respin without that part. hw/qdev.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)