[libvirt] [PATCH libvirt 1/2] domain: add <forward> element for user mode networking
Laine Stump
laine at laine.org
Tue May 15 20:58:52 UTC 2012
On 05/09/2012 08:10 PM, Marc-André Lureau wrote:
> Add element <forward> to add TCP or UDP port redirection from host to
> guest in user mode networking.
> ---
> docs/formatdomain.html.in | 21 ++++
> docs/schemas/domaincommon.rng | 29 +++++
> src/conf/domain_conf.c | 140 ++++++++++++++++++++++
> src/conf/domain_conf.h | 23 ++++
> tests/qemuargv2xmltest.c | 3 +-
> tests/qemuxml2argvdata/qemuxml2argv-net-user.xml | 2 +
> 6 files changed, 217 insertions(+), 1 deletion(-)
This is actually a very common request for users of libvirt's default
network (which connects the guest to a host bridge via a tap interface,
and does NAT via iptables rules), so I think it's very important that
the same functionality be achievable using the same XML with that
networking setup.
I had been thinking that port forwarding should be added to nwfilter
rules, since they deal with iptables rules, and it's also iptables rules
that will be used to add port forwarding. But your use case (user mode
networking) points out that it really should be separated from nwfilter.
Of course, in the case of libvirt virtual networks, we will need to add
both a DNAT rule and a FORWARD rule (which could already be added by
nwfilter).
Another thought I had was that it might be nice to use similar
attributes to nwfilter rules as much as possible, but nwfilter doesn't
have any concept of host vs. guest, so it's a bit limited. However, here
are some suggested names, which are at least closer:
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
Looking ahead to adding support for this with other network types - does
it sound like a reasonable idea to add/remove the rules for it in
networkAllocateActualDevice() and networkReleaseActualDevice()?
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d6e90f1..9c9bbf5 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2268,6 +2268,27 @@
> VMs to have outgoing access.
> </p>
>
> + <p>
> + Port redirections from host to guest can be added by
> + providing <code>forward</code> elements that takes the
> + following attributes:
> + </p>
> +
> + <dl>
> + <dt><code>type</code></dt>
> + <dd>Either <code>tcp</code> (default) or <code>udp</code>.</dd>
> +
> + <dt><code>host_port</code></dt>
> + <dd>Host port to redirect.</dd>
> +
> + <dt><code>guest_port</code></dt>
> + <dd>Guest port to redirect to.</dd>
> +
> + <dt><code>host</code></dt>
> + <dd>IPv4 address to bound the redirection to a specific host
Would there ever be a case where 1) a domain name was used in one or the
other place, or 2) a proxy was used to redirect from IPv6 to IPv6, or
translate between the two? (I suppose the RNG could always be expanded
without change to the attribute name, if this was ever needed).
> + interface.</dd>
> + </dl>
> +
> <pre>
> ...
> <devices>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 34f63c3..740f5af 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1408,6 +1408,35 @@
> <value>user</value>
> </attribute>
> <interleave>
> + <zeroOrMore>
> + <element name="forward">
> + <attribute name="host_port">
> + <ref name="PortNumber"/>
> + </attribute>
> + <attribute name="guest_port">
> + <ref name="PortNumber"/>
> + </attribute>
> + <optional>
> + <attribute name="type">
> + <choice>
> + <value>tcp</value>
> + <value>udp</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> + <attribute name="host">
> + <ref name="ipv4Addr"/>
> + </attribute>
> + </optional>
> + <optional>
> + <attribute name="guest">
> + <ref name="ipv4Addr"/>
> + </attribute>
> + </optional>
> + <empty/>
> + </element>
> + </zeroOrMore>
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:
> +
> + if (nforwards > 0) {
> + int i;
> + if (VIR_ALLOC_N(def->data.user.forwards, nforwards) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + for (i = 0; i < nforwards; i++) {
> + virDomainNetForwardDefPtr fwd =
> + virDomainNetForwardDefParseXML(forwardNodes[i]);
> + if (fwd == NULL)
> + goto error;
> + def->data.user.forwards[def->data.user.nforward++] = fwd;
> + }
> + VIR_FREE(forwardNodes);
> + }
> + break;
> +
> case VIR_DOMAIN_NET_TYPE_LAST:
> break;
> }
> @@ -11413,11 +11514,42 @@ error:
> }
>
> static int
> +virDomainNetForwardDefFormat(virBufferPtr buf,
> + virDomainNetForwardDefPtr def)
> +{
> + const char *type;
> + if (!def)
> + return 0;
> +
> + type = virDomainNetForwardTypeToString(def->type);
> + if (!type) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected net type %d"), def->type);
> + return -1;
> + }
> + virBufferAsprintf(buf, "<forward type='%s'", type);
> +
> + if (def->host_addr)
> + virBufferAsprintf(buf, " host='%s'", def->host_addr);
> +
> + virBufferAsprintf(buf, " host_port='%d'", def->host_port);
> +
> + if (def->guest_addr)
> + virBufferAsprintf(buf, " guest='%s'", def->guest_addr);
> +
> + virBufferAsprintf(buf, " guest_port='%d'", def->guest_port);
> +
> + virBufferAddLit(buf, "/>\n");
> + return 0;
> +}
> +
> +static int
> virDomainNetDefFormat(virBufferPtr buf,
> virDomainNetDefPtr def,
> unsigned int flags)
> {
> const char *type = virDomainNetTypeToString(def->type);
> + int i;
>
> if (!type) {
> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -11517,6 +11649,14 @@ virDomainNetDefFormat(virBufferPtr buf,
> break;
>
> case VIR_DOMAIN_NET_TYPE_USER:
> + virBufferAdjustIndent(buf, 6);
> + for (i = 0; i < def->data.user.nforward; i++) {
> + if (virDomainNetForwardDefFormat(buf, def->data.user.forwards[i]) < 0)
> + return -1;
> + }
> + virBufferAdjustIndent(buf, -6);
> + break;
> +
> case VIR_DOMAIN_NET_TYPE_LAST:
> break;
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 70129fe..7fde05e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -768,6 +768,23 @@ struct _virDomainActualNetDef {
> virNetDevBandwidthPtr bandwidth;
> };
>
> +enum virDomainNetForwardType {
> + VIR_DOMAIN_NET_FORWARD_TYPE_TCP,
> + VIR_DOMAIN_NET_FORWARD_TYPE_UDP,
> +
> + VIR_DOMAIN_NET_FORWARD_TYPE_LAST,
> +};
> +
> +typedef struct _virDomainNetForwardDef virDomainNetForwardDef;
> +typedef virDomainNetForwardDef *virDomainNetForwardDefPtr;
> +struct _virDomainNetForwardDef {
> + int type; /* enum virDomainNetForwardType */
> + char *host_addr;
> + int host_port;
> + char *guest_addr;
> + int guest_port;
> +};
> +
> /* Stores the virtual network interface configuration */
> struct _virDomainNetDef {
> enum virDomainNetType type;
> @@ -821,6 +838,10 @@ struct _virDomainNetDef {
> virDomainHostdevDef def;
> virNetDevVPortProfilePtr virtPortProfile;
> } hostdev;
> + struct {
> + int nforward;
> + virDomainNetForwardDefPtr *forwards;
> + } user;
> } data;
> struct {
> bool sndbuf_specified;
> @@ -1856,6 +1877,7 @@ int virDomainDiskFindControllerModel(virDomainDefPtr def,
> void virDomainControllerDefFree(virDomainControllerDefPtr def);
> void virDomainFSDefFree(virDomainFSDefPtr def);
> void virDomainActualNetDefFree(virDomainActualNetDefPtr def);
> +void virDomainNetForwardDefFree(virDomainNetForwardDefPtr def);
> void virDomainNetDefFree(virDomainNetDefPtr def);
> void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def);
> void virDomainChrDefFree(virDomainChrDefPtr def);
> @@ -2170,6 +2192,7 @@ VIR_ENUM_DECL(virDomainFSAccessMode)
> VIR_ENUM_DECL(virDomainFSWrpolicy)
> VIR_ENUM_DECL(virDomainNet)
> VIR_ENUM_DECL(virDomainNetBackend)
> +VIR_ENUM_DECL(virDomainNetForward)
> VIR_ENUM_DECL(virDomainNetVirtioTxMode)
> VIR_ENUM_DECL(virDomainNetInterfaceLinkState)
> VIR_ENUM_DECL(virDomainChrDevice)
> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
> index 439218e..cf2862b 100644
> --- a/tests/qemuargv2xmltest.c
> +++ b/tests/qemuargv2xmltest.c
> @@ -207,7 +207,8 @@ mymain(void)
> DO_TEST("misc-acpi");
> DO_TEST("misc-no-reboot");
> DO_TEST("misc-uuid");
> - DO_TEST("net-user");
> + /* Fixed in following commit */
> + /* DO_TEST("net-user"); */
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.
> DO_TEST("net-virtio");
> DO_TEST("net-eth");
> DO_TEST("net-eth-ifname");
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml
> index 37e5edf..ecefdeb 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml
> @@ -23,6 +23,8 @@
> <controller type='ide' index='0'/>
> <interface type='user'>
> <mac address='00:11:22:33:44:55'/>
> + <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?
> </interface>
> <memballoon model='virtio'/>
> </devices>
More information about the libvir-list
mailing list