Re: [libvirt] [PATCH libvirt 1/2] domain: add <forward> element for user mode networking

Current      Suggested       similar nwfilter -------      ---------       ---------------- type         protocol        protocol  (I see Dan already suggested this one) host_port    hostport        dstportstart guest_port   guestport       dstportstart host         hostipaddr      dstipaddr guest        guestipaddr     dstipaddr

I think this should be in the common part of <interface> rather than user-specific. This isn't something inherently specific to type='user' (as a matter of fact, I would like to expand it fairly soon after you're done to also work for type='network'), and restricting it as you're doing here will just need to be undone later (including moving things around in the virDomainNetDef struct, etc). I think instead the RNG, parser, and formatter should allow it for all types of interface, and the hypervisor driver should log a VIR_ERR_CONFIG_UNSUPPORTED error and fail to attach if <forward> is given for an interface type that doesn't support it.

>              <ref name="interface-options"/> >            </interleave> >          </group> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 51d6cb9..4b9b644 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -650,6 +650,10 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, >                "static", >                "auto"); > > +VIR_ENUM_IMPL(virDomainNetForward, VIR_DOMAIN_NET_FORWARD_TYPE_LAST, > +              "tcp", > +              "udp") > + >  #define virDomainReportError(code, ...)                              \ >      virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__,            \ >                           __FUNCTION__, __LINE__, __VA_ARGS__) > @@ -1019,8 +1023,22 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) >      VIR_FREE(def); >  } > > +void > +virDomainNetForwardDefFree(virDomainNetForwardDefPtr def) > +{ > +    if (!def) > +        return; > + > +    VIR_FREE(def->host_addr); > +    VIR_FREE(def->guest_addr); > + > +    VIR_FREE(def); > +} > + >  void virDomainNetDefFree(virDomainNetDefPtr def) >  { > +    int i; > + >      if (!def) >          return; > > @@ -1066,6 +1084,11 @@ void virDomainNetDefFree(virDomainNetDefPtr def) >          break; > >      case VIR_DOMAIN_NET_TYPE_USER: > +        for (i = 0; i < def->data.user.nforward; i++) > +            virDomainNetForwardDefFree(def->data.user.forwards[i]); > +        VIR_FREE(def->data.user.forwards); > +        break; > + >      case VIR_DOMAIN_NET_TYPE_LAST: >          break; >      } > @@ -4351,6 +4374,60 @@ error: >      return ret; >  } > > +static virDomainNetForwardDefPtr > +virDomainNetForwardDefParseXML(const xmlNodePtr node) > +{ > +    char *type = NULL; > +    char *host_port = NULL; > +    char *guest_port = NULL; > +    virDomainNetForwardDefPtr def; > + > +    if (VIR_ALLOC(def) < 0) { > +        virReportOOMError(); > +        return NULL; > +    } > + > +    type = virXMLPropString(node, "type"); > +    if (type == NULL) { > +        def->type = VIR_DOMAIN_NET_FORWARD_TYPE_TCP; > +    } else if ((def->type = virDomainNetForwardTypeFromString(type)) < 0) { > +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, > +                             _("unknown forward type '%s'"), type); > +        goto error; If you take this goto, you'll call virDomainNetForwardDefFree on an uninitialized def. You should initialize it to NULL.

> +    } > + > +    host_port = virXMLPropString(node, "host_port"); > +    if (!host_port || > +        virStrToLong_i(host_port, NULL, 10, &def->host_port) < 0) { > +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                             _("Cannot parse <forward> 'host_port' attribute")); > +        goto error; > +    } > + > +    guest_port = virXMLPropString(node, "guest_port"); > +    if (!guest_port || > +        virStrToLong_i(guest_port, NULL, 10, &def->guest_port) < 0) { > +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                             _("Cannot parse <forward> 'guest_port' attribute")); > +        goto error; > +    } > + > +    def->host_addr = virXMLPropString(node, "host"); > +    def->guest_addr = virXMLPropString(node, "guest"); > + > +cleanup: > +    VIR_FREE(type); > +    VIR_FREE(host_port); > +    VIR_FREE(guest_port); > + > +    return def; > + > +error: > +    virDomainNetForwardDefFree(def); > +    def = NULL; > +    goto cleanup; > +} > + >  #define NET_MODEL_CHARS \ >      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" > > @@ -4394,6 +4471,8 @@ virDomainNetDefParseXML(virCapsPtr caps, >      virDomainActualNetDefPtr actual = NULL; >      xmlNodePtr oldnode = ctxt->node; >      int ret; > +    int nforwards; > +    xmlNodePtr *forwardNodes = NULL; > >      if (VIR_ALLOC(def) < 0) { >          virReportOOMError(); > @@ -4683,6 +4762,28 @@ virDomainNetDefParseXML(virCapsPtr caps, >          break; > >      case VIR_DOMAIN_NET_TYPE_USER: > +        /* parse the <forward> elements */ > +        nforwards = virXPathNodeSet("./forward", ctxt, &forwardNodes); > +        if (nforwards < 0) > +            goto error; Since you haven't modified the code down at error:, any error after this point will leak forwardNodes - you need to call VIR_FREE(forwardNodes) at error:

It would be much nicer to make the test work rather than disable it (even for one commit), unless it's really hairy to fix it.

> +      <forward type='tcp' host_port='2222' guest_port='22'/> > +      <forward type='udp' host='127.0.0.1' host_port='2242' guest='10.0.2.15' guest_port='42'/> Ah, I see. You're adding new XML, so you want it represented in the xml2xml tests, but you haven't added the backend, so it will fail the xml2argv test? But since unrecognized XML is just ignored, leaving the argv file unmodified should do it for you, right?