On Fri, 15 Feb 2013 14:03:37 -0500 Laine Stump <laine@laine.org> wrote:
On 02/11/2013 09:54 AM, Natanael Copa wrote:
Let users set the port range to be used for forward mode NAT:
... <forward mode='nat'> <nat> <port start='1024' end='65535'/> </nat> </forward>
...
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 61d086a..5725800 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c
...
@@ -2179,6 +2210,7 @@ virNatDefFormat(virBufferPtr buf, char *addr_start = NULL; char *addr_end = NULL; int ret = -1; + int longdef;
"longdef" must be leftover from an earlier version of your patch, as it's now unused.
Yes. It is a leftover and should be removed. Sorry about that.
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a598e3..7df2426 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -175,8 +175,9 @@ struct _virNetworkForwardDef { size_t nifs; virNetworkForwardIfDefPtr ifs;
- /* adresses for SNAT */ + /* ranges for NAT */ virSocketAddr addr_start, addr_end; + unsigned int port_start, port_end;
As with addr_*, since there is a preference for using Caps instead of _ between words in variable names, I'm going to head this one off at the beginning and change all the port_starts to portStart and portEnds to port_end.
Ok. Sounds good.
};
typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6d74c1f..5c83085 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1589,6 +1589,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1605,6 +1607,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "udp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1621,6 +1625,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "tcp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1639,6 +1645,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "udp"); masqerr4: iptablesRemoveForwardMasquerade(driver->iptables, @@ -1647,6 +1655,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL); masqerr3: iptablesRemoveForwardAllowRelatedIn(driver->iptables, @@ -1679,6 +1689,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "tcp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1686,6 +1698,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end,
There's so many of these, I'm starting to wonder if it's worth having a virPortAddrRange data type (but not wondering enough to actually do it myself :-)
I thought of that too. I think I saw some addr range struct in the code. We could maybe reuse that and add a virPortRange data type. Would reduce half of the forward args.
"udp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1693,6 +1707,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL);
iptablesRemoveForwardAllowRelatedIn(driver->iptables, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 3f0dcf0..aa48520 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -807,6 +807,8 @@ iptablesForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol, int action) { @@ -815,6 +817,7 @@ iptablesForwardMasquerade(iptablesContext *ctx, char *addr_start_str = NULL; char *addr_end_str = NULL; virCommandPtr cmd = NULL; + char port_str[sizeof(":65535-65535")] = "";
Again, port_str (which I'm renaming portRangeStr)
Good.
should be a char* initialized to NULL, and we should call virAsprintf rather than snprintf (with all the associated checks for OOM).
So, it can not be on stack? ...
@@ -849,19 +852,27 @@ iptablesForwardMasquerade(iptablesContext *ctx, if (physdev && physdev[0]) virCommandAddArgList(cmd, "--out-interface", physdev, NULL); + if (protocol && protocol[0]) { + if (port_start == 0 && port_end == 0) { + port_start = 1024; + port_end = 65535; + } + + if (port_start < port_end && port_end < 65536) + snprintf(port_str, sizeof(port_str), ":%d-%d", + port_start, port_end);
1) port_* are unsigned, so the format string should use %u rather than %d
2) If the conditional *isn't* true, that's an error and should be logged.
Ok.
ACK with those nits fixed. I was making them as I went along, and had to rebase the patch anyway, so I'll post a new version that you can review; if you ACK, I'll push.
I am ok with those changes. If you want I could look at the range structs on monday. -nc