On Thu, Oct 29, 2015 at 10:33:51AM -0400, John Ferlan wrote:
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.
Thanks, I'll push it after freeze. Pavel
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list