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

[Daniel Veillard]

On Thu, Feb 11, 2010 at 07:46:59AM -0500, Stefan Berger wrote:
> Daniel Veillard <veillard at redhat.com> 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 at us.ibm.com>
> > [...]
> > > 
> > >  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.

  Okay I was expecting this but wanted to be sure :-)

> > 
> > > +        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.

  okay, thanks, after rebase please :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/