On Tue, Jul 07, 2026 at 12:48:30AM +0200, Stefano Brivio wrote: > On Mon, 6 Jul 2026 13:08:29 +1000 > David Gibson wrote: > > > On Sat, Jul 04, 2026 at 05:27:06PM +0200, Stefano Brivio wrote: > > > On Thu, 2 Jul 2026 17:32:14 +1000 > > > David Gibson wrote: [snip] > > > I wonder: if !addr, shouldn't we replace it, for the moment, with > > > inany_any4 or inany_any6 depending on !!inany_v4(tgt_addr)? > > > > > > I see that an empty address already works with -4 or -6, and I > > > understand that, by doing this, we'll have a change in behaviour once > > > forwarding between IP versions is implemented. > > > > > > But it will take a while before we get there, and, meanwhile, I guess > > > almost all users will just want to do stuff like -t 8080:192.0.2.1/80, > > > just to hit: > > > > > > Forwarding between IP versions (* => 192.0.2.1) not implemented > > > > > > which isn't obvious if you aren't familiar with the implementation. I > > > can also picture a constant flow of incoming tickets as a result. > > > > That's a reasonable concern. I did think about this and came to the > > tentative conclusion that automatically restricting to one family > > would be worse, but I'm open to persuasion otherwise. Reasoning as > > follows. > > > > First, it is a little awkward to implement: I think we should still > > prevent an explicit *:192.0.2.1, which means we'd have to distinguish > > the explicit * and implicit any cases. > > Fair, and I think we should, but that looks relatively simple judging > from fwd_rule_range_except() and callers. Ok. > > Mind you, we probably want > > something similar when we allow listening addresses for -[TU], so it's > > not such a big deal. > > > > More importly this would become the _only_ place where the target > > designation affects where and how we listen - in all other ways, > > that's fully determined by the first part of the spec plus global > > flags like -[46]. This gets particularly striking when looking at > > something like: > > -t 5000,5001:192.0.2.1 > > > > With an automatic restriction, we'd end up listening on *:5000, but > > only on 0.0.0.0:5001, which seems pretty confusing. > > So... I wasn't really aware or the fact that: > > - t x,y > > would always be equivalent to: > > -t x -t y > > as it is now. I originally intended , as a separator for ports and > ports only, but originally we just had ports as port forwarding > specifications. Then forwarding specifiers grew and I missed that, but > it wasn't my intention. Right. Nonetheless, it's been that way for a good long while. > I think that, eventually, we should make -t x_0,...,x_n a mere port > specifier like it already is for ranges, because it's rather intuitive, > as opposed as, say: > > -t 5000 -t 5001:192.0.2.1 > > where it's clear that we want to map port 5001 to 192.0.2.1 as > destination address. If we say -t 5000,5001:192.0.2.1, it's rather > obvious that we do *not*, and yet that's the outcome. Agreed in principle. However, to do it differently we'd need to work out how it interacts with target port specifiers. I don't think the meaning of -t 5000,6000:7000 is particularly clear one way or the other. > On the other hand, "fixing" this is clearly behind the scope of this > series, so, unless you have in mind a quick way to implement it, let's > forget about this, and go ahead with this approach which is consistent > with the existing situation, I'd say. I don't see a quick way to implement it, but more than that it's not really clear to me what our options are for syntax without breaking backwards compatibility too badly. > > > The change in behaviour once we implement forwarding between IP > > > versions, by the way, looks a bit like an extension rather than a real > > > change that could reasonably cause trouble to anybody. > > > > I'd argue that's even more the case with the errors as is: things that > > weren't possible before become possible. > > Indeed. I was just suggesting that even with the other option we would > add an extension at some point, rather than a bug. > > > > > + char bstr[INANY_ADDRSTRLEN]; > > > > + > > > > + die( > > > > +"Forwarding between IP versions (%s => %s) not implemented", > > > > + inany_ntop(addr, bstr, sizeof(bstr)), > > > > + inany_ntop(tgt_addr, astr, sizeof(astr))); > > > > + } > > > > > > About both validations: shouldn't they live in fwd_rule_add(), where > > > we already have this kind of stuff? Or at least be duplicated there? > > > > Yes, yes they should. Which also means it needs to move to the second > > patch (or fwd_add_rule() doesn't see the target address - there's > > nowhere for it in the structure). > > > > For the next spin, I've moved this validation to fwd_rule_add() in > > patch 2. I've also tweaked the messages a bit, which might at least > > slightly mitigate the concerns above. > > Maybe, given that these error messages are temporary, we could indulge > into some extra message like: > > "Did you want to say 0.0.0.0/... instead?" > > same for ::1, along with the rest of the forwarding specifier. Users > would see that when their containers fail to start and they could fix > their syntax right away. Good idea, done. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson