[v3,0/3]

Message

Zhijian Li (Fujitsu)" via Feb. 3, 2021, 11:35 p.m. UTC

Add support for ipv6 host forwarding

This patchset takes the original patch from Maxim,
https://www.mail-archive.com/[e-mail]/msg569573.html
and updates it.

New option: -ipv6-hostfwd

New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove

These are the ipv6 equivalents of their ipv4 counterparts.

The libslirp part of the patch has been committed upstream,
and will require adding it to qemu.
https://gitlab.freedesktop.org/slirp/libslirp/-/commit/0624fbe7d39b5433d7084a5096d1effc1df74e39
[plus the subsequent merge commit]

Changes from v2:
- split out libslirp commit
- clarify spelling of ipv6 addresses in docs
- tighten parsing of ipv6 addresses

Change from v1:
- libslirp part is now upstream
- net/slirp.c changes split into two pieces (refactor, add ipv6)
- added docs

Doug Evans (3):
  slirp: Placeholder for libslirp ipv6 hostfwd support
  net/slirp.c: Refactor address parsing
  net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

 hmp-commands.hx     |  32 +++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 310 +++++++++++++++++++++++++++++++++++---------
 qapi/net.json       |   4 +
 slirp               |   2 +-
 5 files changed, 285 insertions(+), 65 deletions(-)

Comments

[e-mail] Feb. 3, 2021, 11:45 p.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/[e-mail]/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: [e-mail]
Subject: [PATCH v3 0/3]

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/[e-mail] -> patchew/[e-mail]
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
Switched to a new branch 'test'
998dd12 net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands
e6a45e5 net/slirp.c: Refactor address parsing
6faf9a9 slirp: Placeholder for libslirp ipv6 hostfwd support

=== OUTPUT BEGIN ===
1/3 Checking commit 6faf9a93b1cf (slirp: Placeholder for libslirp ipv6 hostfwd support)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 2 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit e6a45e50689f (net/slirp.c: Refactor address parsing)
3/3 Checking commit 998dd12e51aa (net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands)
ERROR: line over 90 characters
#145: FILE: net/slirp.c:748:
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 address", kind);

total: 1 errors, 0 warnings, 250 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/[e-mail]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

Daniel P. Berrangé Feb. 4, 2021, 10:03 a.m. UTC | #2

On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> Add support for ipv6 host forwarding
> 
> This patchset takes the original patch from Maxim,
> https://www.mail-archive.com/[e-mail]/msg569573.html
> and updates it.
> 
> New option: -ipv6-hostfwd
> 
> New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> 
> These are the ipv6 equivalents of their ipv4 counterparts.

Before I noticed this v3, I send a reply to your v2 sugesting
that we don't need to add any new commands/options. We can
use existing inet_parse() helper function to parse the address
info and transparently support IPv4/6 in the existing commands
and options. This matches normal practice elsewhere in QEMU
for IP dual stack.


Regards,
Daniel

Doug Evans Feb. 4, 2021, 6:25 p.m. UTC | #3

On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <[e-mail]>
wrote:

> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > Add support for ipv6 host forwarding
> >
> > This patchset takes the original patch from Maxim,
> > https://www.mail-archive.com/[e-mail]/msg569573.html
> > and updates it.
> >
> > New option: -ipv6-hostfwd
> >
> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >
> > These are the ipv6 equivalents of their ipv4 counterparts.
>
> Before I noticed this v3, I send a reply to your v2 sugesting
> that we don't need to add any new commands/options. We can
> use existing inet_parse() helper function to parse the address
> info and transparently support IPv4/6 in the existing commands
> and options. This matches normal practice elsewhere in QEMU
> for IP dual stack.
>

I'm all for this, fwiw.

Doug Evans Feb. 10, 2021, 2:16 a.m. UTC | #4

On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:

> On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <[e-mail]>
> wrote:
>
>> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
>> > Add support for ipv6 host forwarding
>> >
>> > This patchset takes the original patch from Maxim,
>> > https://www.mail-archive.com/[e-mail]/msg569573.html
>> > and updates it.
>> >
>> > New option: -ipv6-hostfwd
>> >
>> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
>> >
>> > These are the ipv6 equivalents of their ipv4 counterparts.
>>
>> Before I noticed this v3, I send a reply to your v2 sugesting
>> that we don't need to add any new commands/options. We can
>> use existing inet_parse() helper function to parse the address
>> info and transparently support IPv4/6 in the existing commands
>> and options. This matches normal practice elsewhere in QEMU
>> for IP dual stack.
>>
>
> I'm all for this, fwiw.
>


I should say I'm all for not adding new commands/options.
Looking at inet_parse() it cannot be used as-is.
The question then becomes: Will refactoring it buy enough?

Daniel P. Berrangé Feb. 10, 2021, 9:31 a.m. UTC | #5

On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:
> 
> > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <[e-mail]>
> > wrote:
> >
> >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> >> > Add support for ipv6 host forwarding
> >> >
> >> > This patchset takes the original patch from Maxim,
> >> > https://www.mail-archive.com/[e-mail]/msg569573.html
> >> > and updates it.
> >> >
> >> > New option: -ipv6-hostfwd
> >> >
> >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >> >
> >> > These are the ipv6 equivalents of their ipv4 counterparts.
> >>
> >> Before I noticed this v3, I send a reply to your v2 sugesting
> >> that we don't need to add any new commands/options. We can
> >> use existing inet_parse() helper function to parse the address
> >> info and transparently support IPv4/6 in the existing commands
> >> and options. This matches normal practice elsewhere in QEMU
> >> for IP dual stack.
> >>
> >
> > I'm all for this, fwiw.
> >
> 
> 
> I should say I'm all for not adding new commands/options.
> Looking at inet_parse() it cannot be used as-is.
> The question then becomes: Will refactoring it buy enough?

What's the problem your hitting with inet_parse ?

Regards,
Daniel

Doug Evans Feb. 10, 2021, 4:31 p.m. UTC | #6

On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <[e-mail]>
wrote:

> On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:
> >
> > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <[e-mail]
> >
> > > wrote:
> > >
> > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > >> > Add support for ipv6 host forwarding
> > >> >
> > >> > This patchset takes the original patch from Maxim,
> > >> > https://www.mail-archive.com/[e-mail]/msg569573.html
> > >> > and updates it.
> > >> >
> > >> > New option: -ipv6-hostfwd
> > >> >
> > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > >> >
> > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > >>
> > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > >> that we don't need to add any new commands/options. We can
> > >> use existing inet_parse() helper function to parse the address
> > >> info and transparently support IPv4/6 in the existing commands
> > >> and options. This matches normal practice elsewhere in QEMU
> > >> for IP dual stack.
> > >>
> > >
> > > I'm all for this, fwiw.
> > >
> >
> >
> > I should say I'm all for not adding new commands/options.
> > Looking at inet_parse() it cannot be used as-is.
> > The question then becomes: Will refactoring it buy enough?
>
> What's the problem your hitting with inet_parse ?
>


First, this is the inet_parse() function we're talking about, right?

int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)

https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

Daniel P. Berrangé Feb. 10, 2021, 4:49 p.m. UTC | #7

On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <[e-mail]>
> wrote:
> 
> > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:
> > >
> > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <[e-mail]
> > >
> > > > wrote:
> > > >
> > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > >> > Add support for ipv6 host forwarding
> > > >> >
> > > >> > This patchset takes the original patch from Maxim,
> > > >> > https://www.mail-archive.com/[e-mail]/msg569573.html
> > > >> > and updates it.
> > > >> >
> > > >> > New option: -ipv6-hostfwd
> > > >> >
> > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > >> >
> > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > >>
> > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > >> that we don't need to add any new commands/options. We can
> > > >> use existing inet_parse() helper function to parse the address
> > > >> info and transparently support IPv4/6 in the existing commands
> > > >> and options. This matches normal practice elsewhere in QEMU
> > > >> for IP dual stack.
> > > >>
> > > >
> > > > I'm all for this, fwiw.
> > > >
> > >
> > >
> > > I should say I'm all for not adding new commands/options.
> > > Looking at inet_parse() it cannot be used as-is.
> > > The question then becomes: Will refactoring it buy enough?
> >
> > What's the problem your hitting with inet_parse ?
> >
> 
> 
> First, this is the inet_parse() function we're talking about, right?
> 
> int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> 
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

Yes, that's right.

Regards,
Daniel

Doug Evans Feb. 10, 2021, 10:40 p.m. UTC | #8

On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <[e-mail]>
wrote:

> On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <[e-mail]>
> > wrote:
> >
> > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:
> > > >
> > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> [e-mail]
> > > >
> > > > > wrote:
> > > > >
> > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > >> > Add support for ipv6 host forwarding
> > > > >> >
> > > > >> > This patchset takes the original patch from Maxim,
> > > > >> >
> https://www.mail-archive.com/[e-mail]/msg569573.html
> > > > >> > and updates it.
> > > > >> >
> > > > >> > New option: -ipv6-hostfwd
> > > > >> >
> > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > >> >
> > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > >>
> > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > >> that we don't need to add any new commands/options. We can
> > > > >> use existing inet_parse() helper function to parse the address
> > > > >> info and transparently support IPv4/6 in the existing commands
> > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > >> for IP dual stack.
> > > > >>
> > > > >
> > > > > I'm all for this, fwiw.
> > > > >
> > > >
> > > >
> > > > I should say I'm all for not adding new commands/options.
> > > > Looking at inet_parse() it cannot be used as-is.
> > > > The question then becomes: Will refactoring it buy enough?
> > >
> > > What's the problem your hitting with inet_parse ?
> > >
> >
> >
> > First, this is the inet_parse() function we're talking about, right?
> >
> > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >
> >
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
>
> Yes, that's right.
>


Thanks, just wanted to be sure.

The syntax it supports is not the same as what's needed for host forwarding.
inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
hostfwd: address:port-address:port
If we wanted to have a utility that parsed "address:port" for v4+v6 then
we'd have to split the "address:port" part out of inet_parse.

Plus the way inet_parse() parses the address, which is fine for its
purposes, is with sscanf.
Alas the terminating character is not the same (',' vs '-').
IWBN to retain passing sscanf a constant format string so that the compiler
can catch various errors,
and if one keeps that then any kind of refactor loses some appeal.
[Though one could require all callers to accept either ',' or '-' as the
delimiter.]

Daniel P. Berrangé Feb. 11, 2021, 9:12 a.m. UTC | #9

On Wed, Feb 10, 2021 at 02:40:17PM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <[e-mail]>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <[e-mail]>
> > > wrote:
> > >
> > > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <[e-mail]> wrote:
> > > > >
> > > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> > [e-mail]
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > > >> > Add support for ipv6 host forwarding
> > > > > >> >
> > > > > >> > This patchset takes the original patch from Maxim,
> > > > > >> >
> > https://www.mail-archive.com/[e-mail]/msg569573.html
> > > > > >> > and updates it.
> > > > > >> >
> > > > > >> > New option: -ipv6-hostfwd
> > > > > >> >
> > > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > > >> >
> > > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > > >>
> > > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > > >> that we don't need to add any new commands/options. We can
> > > > > >> use existing inet_parse() helper function to parse the address
> > > > > >> info and transparently support IPv4/6 in the existing commands
> > > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > > >> for IP dual stack.
> > > > > >>
> > > > > >
> > > > > > I'm all for this, fwiw.
> > > > > >
> > > > >
> > > > >
> > > > > I should say I'm all for not adding new commands/options.
> > > > > Looking at inet_parse() it cannot be used as-is.
> > > > > The question then becomes: Will refactoring it buy enough?
> > > >
> > > > What's the problem your hitting with inet_parse ?
> > > >
> > >
> > >
> > > First, this is the inet_parse() function we're talking about, right?
> > >
> > > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > >
> > >
> > https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
> >
> > Yes, that's right.
> >
> 
> 
> Thanks, just wanted to be sure.
> 
> The syntax it supports is not the same as what's needed for host forwarding.
> inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
> hostfwd: address:port-address:port
> If we wanted to have a utility that parsed "address:port" for v4+v6 then
> we'd have to split the "address:port" part out of inet_parse.
> 
> Plus the way inet_parse() parses the address, which is fine for its
> purposes, is with sscanf.
> Alas the terminating character is not the same (',' vs '-').
> IWBN to retain passing sscanf a constant format string so that the compiler
> can catch various errors,
> and if one keeps that then any kind of refactor loses some appeal.
> [Though one could require all callers to accept either ',' or '-' as the
> delimiter.]

I think the key useful part to keep common impl for is the handling
of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
"address:port" part out into a new inet_addr_parse() helper that can be
called from inet_pase and from slirp.

inet_parse() can split on the first ",", and then call inet_addr_parse
on the first segment.

slirp can split on "-", and call inet_addr_parse with both segments.


Regards,
Daniel

Doug Evans Feb. 18, 2021, 8:30 p.m. UTC | #10

On Thu, Feb 11, 2021 at 1:12 AM Daniel P. Berrangé <[e-mail]>
wrote:

> [...]
>
> I think the key useful part to keep common impl for is the handling
> of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
> "address:port" part out into a new inet_addr_parse() helper that can be
> called from inet_pase and from slirp.
>
> inet_parse() can split on the first ",", and then call inet_addr_parse
> on the first segment.
>
> slirp can split on "-", and call inet_addr_parse with both segments.
>


v4 here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06011.html