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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list