[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]