On Fri, 15 Feb 2013 14:03:37 -0500
Laine Stump <laine@xxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list