Hi Some of your reviews are already adressed in the new patch: http://www.redhat.com/archives/libvir-list/2012-May/msg00834.html On Tue, May 15, 2012 at 10:58 PM, Laine Stump <laine@xxxxxxxxx> wrote: > 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 already renamed type to protocol. I don't mind renaming the rest of the names for the one you suggested. > > 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. As you say, It can be un-restricted once it is implemented for other types. > > >> <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. I am not sure I follow you. Calling virDomainNetForwardDefFree() after VIR_ALLOC(def) should be okay, no? > >> + } >> + >> + 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: Correct, this needs fixing. > 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? It will fail because the resulting XML isn't the same, iirc. -- Marc-André Lureau -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list