Re: [libvirt] [PATCH v4 2/2] net: add support for specifying port range for forward mode nat

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.

> 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.

> }; > > 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 :-)

> "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)

should be a char* initialized to NULL, and we should call virAsprintf rather than snprintf (with all the associated checks for OOM).

> @@ -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.

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.