[libvirt] [PATCH v2 2/3] virsh-domain: update attach-interface to support type=hostdev

[Pavel Hrdina]

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