| Message ID | 1458473954-18583-1-git-send-email-samuel.thibault@ens-lyon.org |
|---|---|
| State | New |
| Headers | show |
Samuel Thibault, on Sun 20 Mar 2016 12:39:14 +0100, wrote: > void icmp6_init(Slirp *slirp) > { > + if (in6_zero(&slirp->vprefix_addr6)) { > + /* IPv6 is disabled */ > + return; > + } > + (Note: vprefix_addr6 is not actually initialized yet at that point, which poses problem, I'll fix that later, I was more asking for reviewing the principle of the patch itself, the option value, etc.). Samuel
On 20.03.2016 12:39, Samuel Thibault wrote: > Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can > setup IPv4-only and IPv6-only network environments. > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > net/slirp.c | 8 +++++--- > qapi-schema.json | 4 ++-- > qemu-options.hx | 7 ++++--- > slirp/ip6.h | 9 +++++++++ > slirp/ip6_icmp.c | 10 ++++++++++ > slirp/ip6_input.c | 6 ++++++ > slirp/ip_input.c | 5 +++++ > slirp/slirp.c | 5 +++++ > 8 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/net/slirp.c b/net/slirp.c > index 95239bc..3151d4a 100644 > --- a/net/slirp.c > +++ b/net/slirp.c > @@ -244,13 +244,15 @@ static int net_slirp_init(NetClientState *peer, const char *model, > > #if defined(_WIN32) && (_WIN32_WINNT < 0x0600) > /* No inet_pton helper before Vista... */ > - if (vprefix6) { > + if (vprefix6 && strcmp(vprefix6, "::")) { > /* Unsupported */ > return -1; > } > memset(&ip6_prefix, 0, sizeof(ip6_prefix)); > - ip6_prefix.s6_addr[0] = 0xfe; > - ip6_prefix.s6_addr[1] = 0xc0; > + if (!vprefix6) { > + ip6_prefix.s6_addr[0] = 0xfe; > + ip6_prefix.s6_addr[1] = 0xc0; > + } > #else > if (!vprefix6) { > vprefix6 = "fec0::"; > diff --git a/qapi-schema.json b/qapi-schema.json > index 88f9b81..69eb6e7 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2427,7 +2427,7 @@ > # > # @ip: #optional legacy parameter, use net= instead > # > -# @net: #optional IP address and optional netmask > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely > # > # @host: #optional guest-visible address of the host > # > @@ -2443,7 +2443,7 @@ > # @dnssearch: #optional list of DNS suffixes to search, passed as DHCP option > # to the guest > # > -# @ip6-prefix: #optional IPv6 network prefix (default is fec0::) (since 2.6) > +# @ip6-prefix: #optional IPv6 network prefix. Set to :: to disable IPv6 completely (default is fec0::) (since 2.6) The new lines here seem to be longer than 80 characters ... I think you should wrap them. Apart from that, the patch looks fine to me. Thomas
Samuel Thibault <samuel.thibault@ens-lyon.org> writes: > Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can > setup IPv4-only and IPv6-only network environments. Do "net=" and "ip6-net=" mean anything useful? If not, wouldn't that be a more natural way to switch off than abusing the wildcard address? Quick interface review: [...] > diff --git a/qapi-schema.json b/qapi-schema.json > index 88f9b81..69eb6e7 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2427,7 +2427,7 @@ > # > # @ip: #optional legacy parameter, use net= instead > # > -# @net: #optional IP address and optional netmask > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely Long line. Syntax? Default value? > # > # @host: #optional guest-visible address of the host > # > @@ -2443,7 +2443,7 @@ > # @dnssearch: #optional list of DNS suffixes to search, passed as DHCP option > # to the guest > # > -# @ip6-prefix: #optional IPv6 network prefix (default is fec0::) (since 2.6) > +# @ip6-prefix: #optional IPv6 network prefix. Set to :: to disable IPv6 completely (default is fec0::) (since 2.6) Long line. Syntax? (default is fec0::) is now in a confusing spot. Suggest # @ip6-prefix: #optional IPv6 network prefix (default is fec0::) # Set to :: to disable IPv6 completely. # (since 2.6) > # > # @ip6-prefixlen: #optional IPv6 network prefix length (default is 64) (since 2.6) > # > diff --git a/qemu-options.hx b/qemu-options.hx > index 732ed8c..4938213 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1712,8 +1712,8 @@ Assign symbolic name for use in monitor commands. > > @item net=@var{addr}[/@var{mask}] > Set IP network address the guest will see. Optionally specify the netmask, > -either in the form a.b.c.d or as number of valid top-most bits. Default is > -10.0.2.0/24. > +either in the form a.b.c.d or as number of valid top-most bits. Set to 0.0.0.0 > +to disable IPv4 completely. Default is 10.0.2.0/24. Long line. > @item host=@var{addr} > Specify the guest-visible address of the host. Default is the 2nd IP in the > @@ -1721,7 +1721,8 @@ guest network, i.e. x.x.x.2. > > @item ip6-net=@var{addr}[/@var{int}] > Set IPv6 network address the guest will see. Optionally specify the prefix > -size, as number of valid top-most bits. Default is fec0::/64. > +size, as number of valid top-most bits. Set to :: to disable IPv6 completely. > +Default is fec0::/64. > > @item ip6-host=@var{addr} > Specify the guest-visible IPv6 address of the host. Default is the 2nd IPv6 in [...]
Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: > > +either in the form a.b.c.d or as number of valid top-most bits. Set to 0.0.0.0 > > +to disable IPv4 completely. Default is 10.0.2.0/24. > > Long line. How long is too long? This is 78 characters, and I see plenty of lines beyond 72 characters in the file... Samuel
Samuel Thibault <samuel.thibault@gnu.org> writes: > Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: >> > +either in the form a.b.c.d or as number of valid top-most bits. Set to 0.0.0.0 >> > +to disable IPv4 completely. Default is 10.0.2.0/24. >> >> Long line. > > How long is too long? This is 78 characters, and I see plenty of lines > beyond 72 characters in the file... Whoops, this one's still within the documented 80 character limit. Regardless, I recommend to limit English text to 70 characters for readability. That's already a compromise: "For best legibility, typographic manuals suggest that columns should be wide enough to contain roughly 60 characters per line." https://en.wikipedia.org/wiki/Column_%28typography%29#Typographic_style
Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: > Samuel Thibault <samuel.thibault@ens-lyon.org> writes: > > Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can > > setup IPv4-only and IPv6-only network environments. > > Do "net=" and "ip6-net=" mean anything useful? If not, wouldn't that be > a more natural way to switch off than abusing the wildcard address? An empty parameter looks odd to me. 0.0.0.0 is used e.g. by ifconfig to disable an interface, that's why I thought about it. Perhaps an even better way would be net=none and ip6-net=none? > > @@ -2427,7 +2427,7 @@ > > # > > # @ip: #optional legacy parameter, use net= instead > > # > > -# @net: #optional IP address and optional netmask > > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely > > Long line. > > Syntax? Default value? Well, that's what was there :) But yes I can add that along the way. I'm however now wondering what difference is supposed to exist between the documentation in qapi-schema.json and in qemu-options.hx? (I know they are separate software layers, thus the two documentations, but does it make sense to have differing documentations when the qapi schema and the CLI options work the same?) Samuel
Samuel Thibault <samuel.thibault@gnu.org> writes: > Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: >> Samuel Thibault <samuel.thibault@ens-lyon.org> writes: >> > Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can >> > setup IPv4-only and IPv6-only network environments. >> >> Do "net=" and "ip6-net=" mean anything useful? If not, wouldn't that be >> a more natural way to switch off than abusing the wildcard address? > > An empty parameter looks odd to me. 0.0.0.0 is used e.g. by ifconfig to > disable an interface, that's why I thought about it. Perhaps an even > better way would be net=none and ip6-net=none? An empty string as parameter value looks just fine to me. "none" would be fine, too, because it's not a valid value so far. I acknowledge the precedence for abusing the wildcard address. Pick something you like. >> > @@ -2427,7 +2427,7 @@ >> > # >> > # @ip: #optional legacy parameter, use net= instead >> > # >> > -# @net: #optional IP address and optional netmask >> > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely >> >> Long line. >> >> Syntax? Default value? > > Well, that's what was there :) > > But yes I can add that along the way. I'm however now wondering > what difference is supposed to exist between the documentation in > qapi-schema.json and in qemu-options.hx? (I know they are separate > software layers, thus the two documentations, but does it make sense to > have differing documentations when the qapi schema and the CLI options > work the same?) If one of them covers something the other doesn't, chances are there's a doc bug. Perhaps we can some day define the command line language with a QAPI schema, just like we define the QMP language now.
On 22.03.2016 08:41, Markus Armbruster wrote: > Samuel Thibault <samuel.thibault@gnu.org> writes: > >> Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: >>> Samuel Thibault <samuel.thibault@ens-lyon.org> writes: >>>> Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can >>>> setup IPv4-only and IPv6-only network environments. >>> >>> Do "net=" and "ip6-net=" mean anything useful? If not, wouldn't that be >>> a more natural way to switch off than abusing the wildcard address? >> >> An empty parameter looks odd to me. 0.0.0.0 is used e.g. by ifconfig to >> disable an interface, that's why I thought about it. Perhaps an even >> better way would be net=none and ip6-net=none? > > An empty string as parameter value looks just fine to me. "none" would > be fine, too, because it's not a valid value so far. I acknowledge the > precedence for abusing the wildcard address. Pick something you like. I like the "=none" syntax. Thomas
Hello, Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: > > -# @net: #optional IP address and optional netmask > > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely > > Long line. > > Syntax? Default value? Something like this? # @net: #optional IP network address that the guest will see, in the # form addr[/netmask]. The netmask is optional, and can be either in the # form a.b.c.d or as a number of valid top-most bits. Default is # 10.0.2.0/24. Set to 'none' to disable IPv4 completely. > > -# @ip6-prefix: #optional IPv6 network prefix (default is fec0::) (since 2.6) > > +# @ip6-prefix: #optional IPv6 network prefix. Set to :: to disable IPv6 completely (default is fec0::) (since 2.6) > > Syntax? Well, it's just an IPv6 address, is there really something to document? Samuel
Samuel Thibault <samuel.thibault@gnu.org> writes: > Hello, > > Markus Armbruster, on Mon 21 Mar 2016 08:33:52 +0100, wrote: >> > -# @net: #optional IP address and optional netmask >> > +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely >> >> Long line. >> >> Syntax? Default value? > > Something like this? > > # @net: #optional IP network address that the guest will see, in the > # form addr[/netmask]. The netmask is optional, and can be either in the > # form a.b.c.d or as a number of valid top-most bits. Default is > # 10.0.2.0/24. Set to 'none' to disable IPv4 completely. Works for me. >> > -# @ip6-prefix: #optional IPv6 network prefix (default is fec0::) (since 2.6) >> > +# @ip6-prefix: #optional IPv6 network prefix. Set to :: to disable IPv6 completely (default is fec0::) (since 2.6) >> >> Syntax? > > Well, it's just an IPv6 address, is there really something to document? A sufficiently confused reader could perhaps get the idea that a DNS name that resolves to an address was acceptable. Here's my try, feel free to edit to taste: # @ip6-prefix: IPv6 network prefix or 'none' (default fec0::) (since 2.6) # The network prefix is given in the usual hexadecimal IPv6 address # notation. # 'none' disables IPv6 completely.
On Sun, Mar 20, 2016 at 12:39:14PM +0100, Samuel Thibault wrote: > Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can > setup IPv4-only and IPv6-only network environments. I really don't like this kind of magic because it is totally invisible to the user unless they read the docs. With the SocketAddress QAPI schema (that is exposed on the command line via -chardev and -vnc options) we already defined a clear way to disable IPv4/6 via boolean flags ipv4=on|off & ipv6=on|off when users see a boolean 'ipv6' option it is pretty clear what it can do without needing them to read the docs. So can we just use this same syntax for slirp too. Regards, Daniel
diff --git a/net/slirp.c b/net/slirp.c index 95239bc..3151d4a 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -244,13 +244,15 @@ static int net_slirp_init(NetClientState *peer, const char *model, #if defined(_WIN32) && (_WIN32_WINNT < 0x0600) /* No inet_pton helper before Vista... */ - if (vprefix6) { + if (vprefix6 && strcmp(vprefix6, "::")) { /* Unsupported */ return -1; } memset(&ip6_prefix, 0, sizeof(ip6_prefix)); - ip6_prefix.s6_addr[0] = 0xfe; - ip6_prefix.s6_addr[1] = 0xc0; + if (!vprefix6) { + ip6_prefix.s6_addr[0] = 0xfe; + ip6_prefix.s6_addr[1] = 0xc0; + } #else if (!vprefix6) { vprefix6 = "fec0::"; diff --git a/qapi-schema.json b/qapi-schema.json index 88f9b81..69eb6e7 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2427,7 +2427,7 @@ # # @ip: #optional legacy parameter, use net= instead # -# @net: #optional IP address and optional netmask +# @net: #optional IP address and optional netmask. Set to 0.0.0.0 to disable IPv4 completely # # @host: #optional guest-visible address of the host # @@ -2443,7 +2443,7 @@ # @dnssearch: #optional list of DNS suffixes to search, passed as DHCP option # to the guest # -# @ip6-prefix: #optional IPv6 network prefix (default is fec0::) (since 2.6) +# @ip6-prefix: #optional IPv6 network prefix. Set to :: to disable IPv6 completely (default is fec0::) (since 2.6) # # @ip6-prefixlen: #optional IPv6 network prefix length (default is 64) (since 2.6) # diff --git a/qemu-options.hx b/qemu-options.hx index 732ed8c..4938213 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1712,8 +1712,8 @@ Assign symbolic name for use in monitor commands. @item net=@var{addr}[/@var{mask}] Set IP network address the guest will see. Optionally specify the netmask, -either in the form a.b.c.d or as number of valid top-most bits. Default is -10.0.2.0/24. +either in the form a.b.c.d or as number of valid top-most bits. Set to 0.0.0.0 +to disable IPv4 completely. Default is 10.0.2.0/24. @item host=@var{addr} Specify the guest-visible address of the host. Default is the 2nd IP in the @@ -1721,7 +1721,8 @@ guest network, i.e. x.x.x.2. @item ip6-net=@var{addr}[/@var{int}] Set IPv6 network address the guest will see. Optionally specify the prefix -size, as number of valid top-most bits. Default is fec0::/64. +size, as number of valid top-most bits. Set to :: to disable IPv6 completely. +Default is fec0::/64. @item ip6-host=@var{addr} Specify the guest-visible IPv6 address of the host. Default is the 2nd IPv6 in diff --git a/slirp/ip6.h b/slirp/ip6.h index 8ddfa24..da23de6 100644 --- a/slirp/ip6.h +++ b/slirp/ip6.h @@ -26,6 +26,12 @@ 0x00, 0x00, 0x00, 0x00,\ 0x00, 0x00, 0x00, 0x02 } } +#define ZERO_ADDR { .s6_addr = \ + { 0x00, 0x00, 0x00, 0x00,\ + 0x00, 0x00, 0x00, 0x00,\ + 0x00, 0x00, 0x00, 0x00,\ + 0x00, 0x00, 0x00, 0x00 } } + static inline bool in6_equal(const struct in6_addr *a, const struct in6_addr *b) { return memcmp(a, b, sizeof(*a)) == 0; @@ -84,6 +90,9 @@ static inline bool in6_equal_mach(const struct in6_addr *a, #define in6_solicitednode_multicast(a)\ (in6_equal_net(a, &(struct in6_addr)SOLICITED_NODE_PREFIX, 104)) +#define in6_zero(a)\ + (in6_equal(a, &(struct in6_addr)ZERO_ADDR)) + /* Compute emulated host MAC address from its ipv6 address */ static inline void in6_compute_ethaddr(struct in6_addr ip, uint8_t eth[ETH_ALEN]) diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c index 9d61349..69c0a16 100644 --- a/slirp/ip6_icmp.c +++ b/slirp/ip6_icmp.c @@ -24,6 +24,11 @@ static void ra_timer_handler(void *opaque) void icmp6_init(Slirp *slirp) { + if (in6_zero(&slirp->vprefix_addr6)) { + /* IPv6 is disabled */ + return; + } + slirp->ra_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, ra_timer_handler, slirp); timer_mod(slirp->ra_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + NDP_Interval); @@ -31,6 +36,11 @@ void icmp6_init(Slirp *slirp) void icmp6_cleanup(Slirp *slirp) { + if (in6_zero(&slirp->vprefix_addr6)) { + /* IPv6 is disabled */ + return; + } + timer_del(slirp->ra_timer); timer_free(slirp->ra_timer); } diff --git a/slirp/ip6_input.c b/slirp/ip6_input.c index c0b11e7..7801043 100644 --- a/slirp/ip6_input.c +++ b/slirp/ip6_input.c @@ -24,6 +24,12 @@ void ip6_cleanup(Slirp *slirp) void ip6_input(struct mbuf *m) { struct ip6 *ip6; + Slirp *slirp = m->slirp; + + if (in6_zero(&slirp->vprefix_addr6)) { + /* IPv6 is disabled */ + goto bad; + } DEBUG_CALL("ip6_input"); DEBUG_ARG("m = %lx", (long)m); diff --git a/slirp/ip_input.c b/slirp/ip_input.c index b464f6b..a519bf6 100644 --- a/slirp/ip_input.c +++ b/slirp/ip_input.c @@ -80,6 +80,11 @@ ip_input(struct mbuf *m) register struct ip *ip; int hlen; + if (!slirp->vnetwork_addr.s_addr) { + /* IPv4 is disabled */ + goto bad; + } + DEBUG_CALL("ip_input"); DEBUG_ARG("m = %p", m); DEBUG_ARG("m_len = %d", m->m_len); diff --git a/slirp/slirp.c b/slirp/slirp.c index 9ccf415..4a4c79e 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -693,6 +693,11 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) int ar_op; struct ex_list *ex_ptr; + if (!slirp->vnetwork_addr.s_addr) { + /* IPv4 is disabled */ + return; + } + ar_op = ntohs(ah->ar_op); switch(ar_op) { case ARPOP_REQUEST:
Make net=0.0.0.0 disable IPv4 and ip6-net=:: disable IPv6, so the user can setup IPv4-only and IPv6-only network environments. Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- net/slirp.c | 8 +++++--- qapi-schema.json | 4 ++-- qemu-options.hx | 7 ++++--- slirp/ip6.h | 9 +++++++++ slirp/ip6_icmp.c | 10 ++++++++++ slirp/ip6_input.c | 6 ++++++ slirp/ip_input.c | 5 +++++ slirp/slirp.c | 5 +++++ 8 files changed, 46 insertions(+), 8 deletions(-)