Re: [libvirt] [PATCH 3/5] macvtap support for libvirt -- qemu support

Daniel Veillard <veillard(a)redhat.com&gt; wrote on 02/11/2010 05:12:07 AM: > > Please respond to veillard > > On Mon, Feb 08, 2010 at 02:37:18PM -0500, Stefan Berger wrote: > > This part adds support for qemu making a macvtap tap device available > > via file descriptor passed to qemu command line. This also attempts to > > tear down the macvtap device when a VM terminates. This includes support > > for attachment and detachment to/from running VM. > > > > Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com&gt; > [...] > > > > int > > +qemudPhysIfaceConnect(virConnectPtr conn, > > + virDomainNetDefPtr net, > > + char *linkdev, > > + int brmode) > > +{ > > + int rc; > > +#if defined(WITH_MACVTAP) > > + char *res_ifname = NULL; > > + int hasBusyDev = 0; > > + > > + delMacvtapByMACAddress(conn, net->mac, &hasBusyDev); > > + > > + if (hasBusyDev) { > > + virReportSystemError(NULL, errno, "%s", > > + _("A macvtap with the same MAC > address is in use")); > > + return -1; > > + } > > + > > + rc = openMacvtapTap(conn, net->ifname, net->mac, linkdev, brmode, > > + &res_ifname); > > + if (rc > 0) { > > since openMacvtapTap seems to return an fd, rc == 0 sounds a > correct possible return, or am I missing something ? I'll fix this. Should be rc >= 0, with 0 as a possible filedescriptor.

> > > + VIR_FREE(net->ifname); > > + net->ifname = res_ifname; > > + } > > +#else > > + (void)net; > > + (void)linkdev; > > + (void)brmode; > > + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, > > + "%s", _("No support for macvtap device")); > > + rc = -1; > > +#endif > > + return rc; > > +} > > + > > Since qemudPhysIfaceConnect can return -1, 0 or > 0 all with different > semantic, I think a comment describing the function and return value > is in order. Added description of function in upcoming patch. > > > +int > > qemudNetworkIfaceConnect(virConnectPtr conn, > > struct qemud_driver *driver, > > virDomainNetDefPtr net, > > @@ -2520,6 +2558,7 @@ qemuBuildHostNetStr(virConnectPtr conn, > > switch (net->type) { > > case VIR_DOMAIN_NET_TYPE_NETWORK: > > case VIR_DOMAIN_NET_TYPE_BRIDGE: > > + case VIR_DOMAIN_NET_TYPE_DIRECT: > > virBufferAddLit(&buf, "tap"); > > virBufferVSprintf(&buf, "%cfd=%s", type_sep, tapfd); > > type_sep = ','; > > Otherwise looks fine, ACK once resolved :-) Will repost.