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