[libvirt] two hostdev devices problem

[Laine Stump]

On 05/21/2013 07:44 AM, Ján Tomko wrote:
> On 05/21/2013 01:37 PM, Laine Stump wrote:
>> On 05/21/2013 04:03 AM, Ján Tomko wrote:
>>> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
>>>> hi,
>>>> I try to add 2 VF by "hostdev".
>>>> Networks (vnet0, vnet1) with:
>>>>  <forward mode='hostdev' managed='yes'>
>>>>     <pf dev='eth1'/>
>>>> .....
>>>>
>>>> Domain:                 
>>>> <interface type="network">
>>>>              <source network="vnet0"/>
>>>> ....
>>>> <interface type="network">
>>>>              <source network="vnet1"/>
>>>> ....
>>>>
>>>> virsh create error:
>>>> "error: internal error process exited while connecting to monitor: kvm:
>>>> -device pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
>>>> Duplicate ID 'hostdev0' for device"
>>>>
>>> Hi,
>>>
>>> it seems we have been assigning the same id to all network hostdevs until this
>>> recent commit (not yet released):
>>>
>>> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
>>
>> If that patch has such an effect, it's purely accidental. Have you
>> tested this? (I plan to test a before and after as soon as I've had
>> breakfast)
>>
> I haven't tested it and looking at the code again, I might've been wrong :(
>
> Sorry about that.

Nothing to be sorry about even if you weren't right. And as it turns out
you *were* right!

I was curious why, so I tested with a 1.0.4 build running under gdb. The
reason for the failure was that the person who wrote the code in
qemuBuildCommandLine that called qemuAssignDeviceHostdevAlias() (as well
as the person who reviewed that code, which would be me :-/) missed a
detail of qemuAssignDeviceHostdevAlias() - it only searches for a free
alias index if you send -1 as the index, otherwise it just uses the
index you send. This is what was being called:

  qemuAssignDeviceHostdevAlias(def, hostdev, (def->nhostdevs-1))

So the first time def->nhostdevs-1 == -1, and a search would be made in
the (empty) hostdevs array, resulting in an index of 0 being used. The
second time it's called, def->nhostdevs-1 == 0, so it would just use 0
without even checking for a conflict.

My patch for VFIO (the one Jan pointed out as being a fix) completely
removed this code from qemuBuildCommandLine(), relying instead on the
alias being assigned with all other aliases in
qemuAssignDeviceAliases(). (I didn't do this on purpose to fix this bug
though; I did it because the old code had become redundant).

Anyway, thanks for being so observant. I otherwise would have wasted
investigation time wondering why it wasn't broken for me.