Patchwork qdev: Reject duplicate and anti-social device IDs

login
register
mail settings
Submitter Markus Armbruster
Date May 31, 2010, 2:13 p.m.
Message ID <m3d3wcrwg7.fsf_-_@blackfin.pond.sub.org>
Download mbox | patch
Permalink /patch/54098/
State New
Headers show

Comments

Markus Armbruster - May 31, 2010, 2:13 p.m.
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(-)
Gerd Hoffmann - May 31, 2010, 6:55 p.m.
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
Luiz Capitulino - June 1, 2010, 1:04 p.m.
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"}}}
Jan Kiszka - June 1, 2010, 1:09 p.m.
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
Luiz Capitulino - June 1, 2010, 1:13 p.m.
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.
Jan Kiszka - June 1, 2010, 1:19 p.m.
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
Avi Kivity - June 1, 2010, 1:21 p.m.
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.
Luiz Capitulino - June 1, 2010, 1:23 p.m.
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.
Markus Armbruster - June 1, 2010, 2:44 p.m.
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.
Luiz Capitulino - June 1, 2010, 2:49 p.m.
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"}}}
Markus Armbruster - June 1, 2010, 6:35 p.m.
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.
Anthony Liguori - June 1, 2010, 6:54 p.m.
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
Jan Kiszka - June 3, 2010, 6:26 a.m.
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
Paul Brook - June 3, 2010, 6:51 a.m.
> 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
Markus Armbruster - June 4, 2010, 2:27 p.m.
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 - June 4, 2010, 3:28 p.m.
> 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.
Markus Armbruster - June 8, 2010, 12:06 p.m.
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.

[...]

Patch

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) {