Re: [libvirt] [PATCH] qemu: allocate network connections sooner during domain startup
On 05/07/2013 10:57 AM, Michal Privoznik wrote:
On 07.05.2013 16:47, Laine Stump wrote:
> On 05/07/2013 05:52 AM, Michal Privoznik wrote:
>> On 06.05.2013 22:16, Laine Stump wrote:
>>>
>> I'd expect a one line comment here at least to give a reason why we are
>> skipping VIR_DOMAIN_NET_TYPE_HOSTDEV. Something like:
>> /* hostdev interfaces already handled by qemuNetworkPrepareDevices */
>>
>> It's obvious now, but if we get later to this code it won't be IMO.
>
> Makes sense. The dual nature of these device does tend to be confusing.
>
>
>
>>> + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV)
>>> + continue;
>>> +
>>> /* VLANs are not used with -netdev, so don't record them */
>>> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
>>> virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
>> I actually posted a patch which moves the whole block into a separate
>> function, which is what we should do:
>>
>> https://www.redhat.com/archives/libvir-list/2013-April/msg01550.html
>
> Ah. I missed that whole series while I was busy trying to get VFIO
> working before the 1.0.5 freeze. I should go back now and review it.
>
>
>
>>> + /* network devices must be "prepared" before hostdevs,
because
>>> + * setting up a network device might create a new hostdev that
>>> + * will need to be setup.
>>> + */
>>> + VIR_DEBUG("Preparing network devices");
>> Is it worth mentioning s/network/network host/?
>
> My intent is the qemuNetworkPrepareDevices() will eventually handle the
> setup required for *all* guest network devices, not just those that are
> passed-through host devices. The only reason I didn't do it that was
> right away is because the fd's for tap and vhost-net devices are
> required in qemuBuildCommandLine(), so we need to decide on the best
> place to stow those away where qemuBuildCommandLine() can retrieve them.
Right, I don't feel comfortable allocating resources in
qemuBuildCommandLine neither.
Among other things, it makes it impossible to test commandline creation
for certain XML - all that extra device creation etc. is an unadvertised
side effect.
So maybe my patch set becomes outdated
after all.
I haven't looked at the rest of it yet, but I think it still makes sense
to put the commandline building for interfaces (and for all other types
of devices) into a separate function - qemuBuildCommandLine is just too
long.
However, if we are gonna move all the allocation code out of
the qemuBuildCommandLine we should remember now, that there are gonna be
several FDs being passed (multiple for /dev/net/tun and for /dev/vhost-net).
Yep. That's the thing (along with fear of an undetected regression) that
prevented me from just doing that in this patch. There isn't really a
convenient place for passing all of those back from some future
qemuNetworkPrepareDevices() to qemuProcessStart(), and from there to
qemuBuildCommandLine() (unless we overload the use of virDomainNetDef or
maybe virDomainActualNetDef, and I'm trying to look for other solutions
before falling back to that).