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

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(a)redhat.com&gt; > --- > 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.

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 with the adjustment to check whether managed is supplied for non hostdev, then you have an ACK for what's changed here.

John