Re: [libvirt] [PATCH v2 2/3] virsh-domain: update attach-interface to support type=hostdev
On 10/29/2015 03:08 AM, Pavel Hrdina wrote:
On Tue, Oct 27, 2015 at 06:16:33PM -0400, John Ferlan wrote:
>
[...]
>
> What happens if someone provides --managed with some other 'typ'?
>
> IOW: If it's only being supported by <hostdev>, then after the switch, a
> check should probably occur for "if (managed && typ !=
> VIR_DOMAIN_NET_TYPE_HOSTDEV), message, and fail.
The check was there, but then I removed it because other arguments doesn't check
the usability too. We don't need to error out, because libvirt just ignore
the "managed='yes'" in the XML.
>
> I'm not fully clear after reading the bz and the previous review whether
> this patch resolves the bz - something to be worked out in the bz for
> history's sake though
I think, that the main issue with the BZ is that there was no easy way how to
attach *hostdev* interface without manually creating the XML for that interface.
We established with Laine, that there is not need for translating netdev name to
PCI address.
>
> I think with the adjustment to check whether managed is supplied for non
> hostdev, then you have an ACK for what's changed here.
Reconsider the 'managed' check, we can be strict and check whether it's used
only with hosted type or not, but it's not necessary.
As I read the docs/code, I see managed is only parsed for <hostdev>
types, so yes from that aspect you're correct. I usually err on the side
of the extra check so that if some day the parsing code gets changed you
don't run into issues. The formatting code certainly only writes out
managed='yes' if type is hostdev, so we're safe from the issue of
managed='yes' being written into the guest XML... I guess it's the
longer way of say ACK for what's here unless you want to be extra paranoid.
John