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

[Marc-André Lureau]

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 at laine.org> 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