[libvirt] two hostdev devices problem

[Daniel P. Berrange]

On Tue, May 21, 2013 at 11:29:26AM -0400, Laine Stump wrote:
> 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.

So the fact that we missed this suggests we have a gap in our XML->ARGV
test coverage for host device assignment, right ?


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|