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

[Pavel Hrdina]

On Tue, Oct 27, 2015 at 06:16:33PM -0400, John Ferlan wrote:
> 
> 
> On 10/27/2015 11:01 AM, Pavel Hrdina wrote:
> > Adding this feature will allow users to easily attach a hostdev network
> > interface using PCI passthrough.
> > 
> > The interface can be attached using --type=hostdev and PCI address or
> > as --source.  This command also allows you to tell, whether the interface
> > should be managed.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997561
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  tools/virsh-domain.c | 34 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 12e85e3..bd00785 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -56,6 +56,7 @@
> >  #include "virtime.h"
> >  #include "virtypedparam.h"
> >  #include "virxml.h"
> > +#include "virsh-nodedev.h"
> >  
> >  /* Gnulib doesn't guarantee SA_SIGINFO support.  */
> >  #ifndef SA_SIGINFO
> > @@ -866,6 +867,10 @@ static const vshCmdOptDef opts_attach_interface[] = {
> >       .type = VSH_OT_BOOL,
> >       .help = N_("print XML document rather than attach the interface")
> >      },
> > +    {.name = "managed",
> > +     .type = VSH_OT_BOOL,
> > +     .help = N_("libvirt will automatically detach/attach the device from/to host")
> > +    },
> >      {.name = NULL}
> >  };
> >  
> > @@ -931,6 +936,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> >      bool config = vshCommandOptBool(cmd, "config");
> >      bool live = vshCommandOptBool(cmd, "live");
> >      bool persistent = vshCommandOptBool(cmd, "persistent");
> > +    bool managed = vshCommandOptBool(cmd, "managed");
> >  
> >      VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
> >  
> > @@ -983,7 +989,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> >      }
> >  
> >      /* Make XML of interface */
> > -    virBufferAsprintf(&buf, "<interface type='%s'>\n", type);
> > +    virBufferAsprintf(&buf, "<interface type='%s'", type);
> > +
> > +    if (managed)
> > +        virBufferAddLit(&buf, " managed='yes'>\n");
> > +    else
> > +        virBufferAddLit(&buf, ">\n");
> >      virBufferAdjustIndent(&buf, 2);
> >  
> >      switch (typ) {
> > @@ -995,6 +1006,26 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> >      case VIR_DOMAIN_NET_TYPE_DIRECT:
> >          virBufferAsprintf(&buf, "<source dev='%s'/>\n", source);
> >          break;
> > +    case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> > +    {
> > +        struct PCIAddress pciAddr = {0, 0, 0, 0};
> > +
> > +        if (str2PCIAddress(source, &pciAddr) < 0) {
> > +            vshError(ctl, _("cannot parse pci address '%s' for network "
> > +                            "interface"), source);
> > +            goto cleanup;
> > +        }
> > +
> > +        virBufferAddLit(&buf, "<source>\n");
> > +        virBufferAdjustIndent(&buf, 2);
> > +        virBufferAsprintf(&buf, "<address type='pci' domain='0x%.4x'"
> > +                          " bus='0x%.2x' slot='0x%.2x' function='0x%.1x'/>\n",
> > +                          pciAddr.domain, pciAddr.bus,
> > +                          pciAddr.slot, pciAddr.function);
> > +        virBufferAdjustIndent(&buf, -2);
> > +        virBufferAddLit(&buf, "</source>\n");
> > +        break;
> > +    }
> >  
> >      case VIR_DOMAIN_NET_TYPE_USER:
> >      case VIR_DOMAIN_NET_TYPE_ETHERNET:
> > @@ -1004,7 +1035,6 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> >      case VIR_DOMAIN_NET_TYPE_MCAST:
> >      case VIR_DOMAIN_NET_TYPE_UDP:
> >      case VIR_DOMAIN_NET_TYPE_INTERNAL:
> > -    case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> >      case VIR_DOMAIN_NET_TYPE_LAST:
> >          vshError(ctl, _("No support for %s in command 'attach-interface'"),
> >                   type);
> > 
> 
> 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.

Thanks,

Pavel

> 
> John