[PATCH] net: Add "info neighbors" command

Doug Evans posted 1 patch 2 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210902212015.1303865-1-dje@google.com
Maintainers: Wainer dos Santos Moschetta <wainersm@redhat.com>, Samuel Thibault <samuel.thibault@ens-lyon.org>, Jason Wang <jasowang@redhat.com>, Cleber Rosa <crosa@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>
hmp-commands-info.hx               | 15 +++++++
include/net/slirp.h                |  1 +
net/slirp.c                        | 15 +++++++
tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
create mode 100644 tests/acceptance/info_neighbors.py
[PATCH] net: Add "info neighbors" command
Posted by Doug Evans 2 years, 6 months ago
This command dumps the ARP and NDP tables maintained within slirp.
One use-case for it is showing the guest's IPv6 address(es).

Signed-off-by: Doug Evans <dje@google.com>
---
 hmp-commands-info.hx               | 15 +++++++
 include/net/slirp.h                |  1 +
 net/slirp.c                        | 15 +++++++
 tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+)
 create mode 100644 tests/acceptance/info_neighbors.py

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 27206ac049..386f09f163 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -512,6 +512,21 @@ SRST
     Show user network stack connection states.
 ERST
 
+#if defined(CONFIG_SLIRP)
+    {
+        .name       = "neighbors",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show the ARP and NDP tables",
+        .cmd        = hmp_info_neighbors,
+    },
+#endif
+
+SRST
+  ``info neighbors``
+    Show the ARP and NDP tables.
+ERST
+
     {
         .name       = "migrate",
         .args_type  = "",
diff --git a/include/net/slirp.h b/include/net/slirp.h
index bad3e1e241..b9ccfda1e7 100644
--- a/include/net/slirp.h
+++ b/include/net/slirp.h
@@ -31,6 +31,7 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
 
 void hmp_info_usernet(Monitor *mon, const QDict *qdict);
+void hmp_info_neighbors(Monitor *mon, const QDict *qdict);
 
 #endif
 
diff --git a/net/slirp.c b/net/slirp.c
index ad3a838e0b..29a4cd3fe1 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -1028,6 +1028,21 @@ void hmp_info_usernet(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_info_neighbors(Monitor *mon, const QDict *qdict)
+{
+    SlirpState *s;
+
+    QTAILQ_FOREACH(s, &slirp_stacks, entry) {
+        int id;
+        bool got_hub_id = net_hub_id_for_client(&s->nc, &id) == 0;
+        char *info = slirp_neighbor_info(s->slirp);
+        monitor_printf(mon, "Hub %d (%s):\n%s",
+                       got_hub_id ? id : -1,
+                       s->nc.name, info);
+        g_free(info);
+    }
+}
+
 static void
 net_init_slirp_configs(const StringList *fwd, int flags)
 {
diff --git a/tests/acceptance/info_neighbors.py b/tests/acceptance/info_neighbors.py
new file mode 100644
index 0000000000..ff79ec3ff3
--- /dev/null
+++ b/tests/acceptance/info_neighbors.py
@@ -0,0 +1,69 @@
+# Test for the hmp command "info neighbors"
+#
+# Copyright 2021 Google LLC
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import re
+
+from avocado_qemu import LinuxTest
+from avocado_qemu import Test
+
+VNET_HUB_HEADER = 'Hub -1 (vnet):'
+NEIGHBOR_HEADER_REGEX = '^ *Table *MacAddr *IP Address$'
+
+def trim(text):
+    return " ".join(text.split())
+
+def hmc(test, cmd):
+    return test.vm.command('human-monitor-command', command_line=cmd)
+
+def get_neighbors(test):
+    output = hmc(test, 'info neighbors').splitlines()
+    if len(output) < 2:
+        test.fail("Insufficient output from 'info neighbors'")
+    test.assertEquals(output[0], VNET_HUB_HEADER)
+    test.assertTrue(re.fullmatch(NEIGHBOR_HEADER_REGEX, output[1]))
+    return output[2:]
+
+class InfoNeighborsNone(Test):
+
+    def test_no_neighbors(self):
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        neighbors = get_neighbors(self)
+        self.assertEquals(len(neighbors), 0)
+
+class InfoNeighbors(LinuxTest):
+
+    def test_neighbors(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=machine:pc
+        :avocado: tags=accel:kvm
+        """
+        self.require_accelerator('kvm')
+        self.vm.add_args("-accel", "kvm")
+        self.vm.add_args('-nographic',
+                         '-m', '1024')
+        self.launch_and_wait()
+
+        # Ensure there's some packets to the guest and back.
+        self.ssh_command('pwd')
+
+        # We should now be aware of the guest as a neighbor.
+        expected_ipv4_neighbor = 'ARP 52:54:00:12:34:56 10.0.2.15'
+        # The default ipv6 net is fec0. Both fe80 and fec0 can appear.
+        expected_ipv6_neighbors = [
+            'NDP 52:54:00:12:34:56 fe80::5054:ff:fe12:3456',
+            'NDP 52:54:00:12:34:56 fec0::5054:ff:fe12:3456'
+        ]
+        neighbors = get_neighbors(self)
+        self.assertTrue(len(neighbors) >= 2 and len(neighbors) <= 3)
+        # IPv4 is output first.
+        self.assertEquals(trim(neighbors[0]), expected_ipv4_neighbor)
+        for neighbor in neighbors[1:]:
+            self.assertTrue(trim(neighbor) in expected_ipv6_neighbors)
-- 
2.33.0.153.gba50c8fa24-goog


Re: [PATCH] net: Add "info neighbors" command
Posted by Markus Armbruster 2 years, 6 months ago
Doug Evans <dje@google.com> writes:

> This command dumps the ARP and NDP tables maintained within slirp.
> One use-case for it is showing the guest's IPv6 address(es).
>
> Signed-off-by: Doug Evans <dje@google.com>
> ---
>  hmp-commands-info.hx               | 15 +++++++
>  include/net/slirp.h                |  1 +
>  net/slirp.c                        | 15 +++++++
>  tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+)
>  create mode 100644 tests/acceptance/info_neighbors.py

Standard request for new HMP commands without corresponding QMP
commands: please state in the commit message why the QMP command is not
worthwhile.

HMP commands without a QMP equivalent are okay if their functionality
makes no sense in QMP, or is of use only for human users.

Example for "makes no sense in QMP": setting the current CPU, because a
QMP monitor doesn't have a current CPU.

Examples for "is of use only for human users": HMP command "help", the
integrated pocket calculator.

Debugging commands are kind of borderline.  Debugging is commonly a
human activity, where HMP is just fine.  However, humans create tools to
assist with their activities, and then QMP is useful.  While I wouldn't
encourage HMP-only for the debugging use case, I wouldn't veto it.


Re: [PATCH] net: Add "info neighbors" command
Posted by Doug Evans 2 years, 6 months ago
On Fri, Sep 3, 2021 at 6:08 AM Markus Armbruster <armbru@redhat.com> wrote:

> Doug Evans <dje@google.com> writes:
>
> > This command dumps the ARP and NDP tables maintained within slirp.
> > One use-case for it is showing the guest's IPv6 address(es).
> >
> > Signed-off-by: Doug Evans <dje@google.com>
> > ---
> >  hmp-commands-info.hx               | 15 +++++++
> >  include/net/slirp.h                |  1 +
> >  net/slirp.c                        | 15 +++++++
> >  tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
> >  4 files changed, 100 insertions(+)
> >  create mode 100644 tests/acceptance/info_neighbors.py
>
> Standard request for new HMP commands without corresponding QMP
> commands: please state in the commit message why the QMP command is not
> worthwhile.
>
> HMP commands without a QMP equivalent are okay if their functionality
> makes no sense in QMP, or is of use only for human users.
>
> Example for "makes no sense in QMP": setting the current CPU, because a
> QMP monitor doesn't have a current CPU.
>
> Examples for "is of use only for human users": HMP command "help", the
> integrated pocket calculator.
>
> Debugging commands are kind of borderline.  Debugging is commonly a
> human activity, where HMP is just fine.  However, humans create tools to
> assist with their activities, and then QMP is useful.  While I wouldn't
> encourage HMP-only for the debugging use case, I wouldn't veto it.
>


Mostly I was following what I saw for "info usernet".
I don't see a difference between "info neighbors" and "info usernet" so I
went with that.
Both draw their data from libslirp.

I'm happy to add QMP support if necessary.
Note that there is code that parses "info usernet" output, e.g.,
get_info_usernet_hostfwd_port for python.

Presumably we don't want to print text in slirp only to parse it in qemu,
right?
That'll change the qemu/slirp interface.
OTOH, to what extent does libslirp want to export a more formal API for
this, vs just text?
Re: [PATCH] net: Add "info neighbors" command
Posted by Markus Armbruster 2 years, 6 months ago
Doug Evans <dje@google.com> writes:

> On Fri, Sep 3, 2021 at 6:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Doug Evans <dje@google.com> writes:
>>
>> > This command dumps the ARP and NDP tables maintained within slirp.
>> > One use-case for it is showing the guest's IPv6 address(es).
>> >
>> > Signed-off-by: Doug Evans <dje@google.com>
>> > ---
>> >  hmp-commands-info.hx               | 15 +++++++
>> >  include/net/slirp.h                |  1 +
>> >  net/slirp.c                        | 15 +++++++
>> >  tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
>> >  4 files changed, 100 insertions(+)
>> >  create mode 100644 tests/acceptance/info_neighbors.py
>>
>> Standard request for new HMP commands without corresponding QMP
>> commands: please state in the commit message why the QMP command is not
>> worthwhile.
>>
>> HMP commands without a QMP equivalent are okay if their functionality
>> makes no sense in QMP, or is of use only for human users.
>>
>> Example for "makes no sense in QMP": setting the current CPU, because a
>> QMP monitor doesn't have a current CPU.
>>
>> Examples for "is of use only for human users": HMP command "help", the
>> integrated pocket calculator.
>>
>> Debugging commands are kind of borderline.  Debugging is commonly a
>> human activity, where HMP is just fine.  However, humans create tools to
>> assist with their activities, and then QMP is useful.  While I wouldn't
>> encourage HMP-only for the debugging use case, I wouldn't veto it.
>>
>
>
> Mostly I was following what I saw for "info usernet".
> I don't see a difference between "info neighbors" and "info usernet" so I
> went with that.
> Both draw their data from libslirp.

I see.

> I'm happy to add QMP support if necessary.
> Note that there is code that parses "info usernet" output, e.g.,
> get_info_usernet_hostfwd_port for python.

Demonstrates "is of use only for human users" is wrong.

> Presumably we don't want to print text in slirp only to parse it in qemu,
> right?

Yes, we'd prefer not to parse.

As long as libslirp can only give us text, we need to parse it
somewhere.

We can parse it right in QEMU, or punt the job to whatever uses QEMU.
The latter can get away with parsing just the part they need.  But we
may end up with multiple parsers.


> That'll change the qemu/slirp interface.
> OTOH, to what extent does libslirp want to export a more formal API for
> this, vs just text?

This is a question for Samuel or Marc-André.

If the answer is "no", then HMP only (so we don't have to parse in QEMU)
is fair, I think.  The commit message should explain this.


Re: [PATCH] net: Add "info neighbors" command
Posted by Marc-André Lureau 2 years, 6 months ago
Hi

On Sat, Sep 4, 2021 at 10:26 AM Markus Armbruster <armbru@redhat.com> wrote:

> Doug Evans <dje@google.com> writes:
>
> > On Fri, Sep 3, 2021 at 6:08 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Doug Evans <dje@google.com> writes:
> >>
> >> > This command dumps the ARP and NDP tables maintained within slirp.
> >> > One use-case for it is showing the guest's IPv6 address(es).
> >> >
> >> > Signed-off-by: Doug Evans <dje@google.com>
> >> > ---
> >> >  hmp-commands-info.hx               | 15 +++++++
> >> >  include/net/slirp.h                |  1 +
> >> >  net/slirp.c                        | 15 +++++++
> >> >  tests/acceptance/info_neighbors.py | 69
> ++++++++++++++++++++++++++++++
> >> >  4 files changed, 100 insertions(+)
> >> >  create mode 100644 tests/acceptance/info_neighbors.py
> >>
> >> Standard request for new HMP commands without corresponding QMP
> >> commands: please state in the commit message why the QMP command is not
> >> worthwhile.
> >>
> >> HMP commands without a QMP equivalent are okay if their functionality
> >> makes no sense in QMP, or is of use only for human users.
> >>
> >> Example for "makes no sense in QMP": setting the current CPU, because a
> >> QMP monitor doesn't have a current CPU.
> >>
> >> Examples for "is of use only for human users": HMP command "help", the
> >> integrated pocket calculator.
> >>
> >> Debugging commands are kind of borderline.  Debugging is commonly a
> >> human activity, where HMP is just fine.  However, humans create tools to
> >> assist with their activities, and then QMP is useful.  While I wouldn't
> >> encourage HMP-only for the debugging use case, I wouldn't veto it.
> >>
> >
> >
> > Mostly I was following what I saw for "info usernet".
> > I don't see a difference between "info neighbors" and "info usernet" so I
> > went with that.
> > Both draw their data from libslirp.
>
> I see.
>
> > I'm happy to add QMP support if necessary.
> > Note that there is code that parses "info usernet" output, e.g.,
> > get_info_usernet_hostfwd_port for python.
>
> Demonstrates "is of use only for human users" is wrong.
>
> > Presumably we don't want to print text in slirp only to parse it in qemu,
> > right?
>
> Yes, we'd prefer not to parse.
>
> As long as libslirp can only give us text, we need to parse it
> somewhere.
>
> We can parse it right in QEMU, or punt the job to whatever uses QEMU.
> The latter can get away with parsing just the part they need.  But we
> may end up with multiple parsers.
>
>
> > That'll change the qemu/slirp interface.
> > OTOH, to what extent does libslirp want to export a more formal API for
> > this, vs just text?
>
> This is a question for Samuel or Marc-André.
>
> If the answer is "no", then HMP only (so we don't have to parse in QEMU)
> is fair, I think.  The commit message should explain this.
>
>
Until now, libslirp (& QEMU HMP) provided inner state details as textual
form. But since there is a need for a programming-friendly API, it could be
added to libslirp and exported as QAPI/QMP by QEMU. Let's try to design the
API in a way that can be easily extended (via getters and/or keys etc).
Alternatively, libslirp could dump its state as JSON, but this wouldn't fit
so nicely with QMP machinery/introspection etc.
Re: [PATCH] net: Add "info neighbors" command
Posted by Patrick Venture 2 years, 6 months ago
On Thu, Sep 2, 2021 at 2:21 PM Doug Evans <dje@google.com> wrote:

> This command dumps the ARP and NDP tables maintained within slirp.
> One use-case for it is showing the guest's IPv6 address(es).
>
> Signed-off-by: Doug Evans <dje@google.com>
>
Reviewed-by: Patrick Venture <venture@google.com>

> ---
>  hmp-commands-info.hx               | 15 +++++++
>  include/net/slirp.h                |  1 +
>  net/slirp.c                        | 15 +++++++
>  tests/acceptance/info_neighbors.py | 69 ++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+)
>  create mode 100644 tests/acceptance/info_neighbors.py
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 27206ac049..386f09f163 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -512,6 +512,21 @@ SRST
>      Show user network stack connection states.
>  ERST
>
> +#if defined(CONFIG_SLIRP)
> +    {
> +        .name       = "neighbors",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the ARP and NDP tables",
> +        .cmd        = hmp_info_neighbors,
> +    },
> +#endif
> +
> +SRST
> +  ``info neighbors``
> +    Show the ARP and NDP tables.
> +ERST
> +
>      {
>          .name       = "migrate",
>          .args_type  = "",
> diff --git a/include/net/slirp.h b/include/net/slirp.h
> index bad3e1e241..b9ccfda1e7 100644
> --- a/include/net/slirp.h
> +++ b/include/net/slirp.h
> @@ -31,6 +31,7 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
>  void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
>
>  void hmp_info_usernet(Monitor *mon, const QDict *qdict);
> +void hmp_info_neighbors(Monitor *mon, const QDict *qdict);
>
>  #endif
>
> diff --git a/net/slirp.c b/net/slirp.c
> index ad3a838e0b..29a4cd3fe1 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -1028,6 +1028,21 @@ void hmp_info_usernet(Monitor *mon, const QDict
> *qdict)
>      }
>  }
>
> +void hmp_info_neighbors(Monitor *mon, const QDict *qdict)
> +{
> +    SlirpState *s;
> +
> +    QTAILQ_FOREACH(s, &slirp_stacks, entry) {
> +        int id;
> +        bool got_hub_id = net_hub_id_for_client(&s->nc, &id) == 0;
> +        char *info = slirp_neighbor_info(s->slirp);
> +        monitor_printf(mon, "Hub %d (%s):\n%s",
> +                       got_hub_id ? id : -1,
> +                       s->nc.name, info);
> +        g_free(info);
> +    }
> +}
> +
>  static void
>  net_init_slirp_configs(const StringList *fwd, int flags)
>  {
> diff --git a/tests/acceptance/info_neighbors.py
> b/tests/acceptance/info_neighbors.py
> new file mode 100644
> index 0000000000..ff79ec3ff3
> --- /dev/null
> +++ b/tests/acceptance/info_neighbors.py
> @@ -0,0 +1,69 @@
> +# Test for the hmp command "info neighbors"
> +#
> +# Copyright 2021 Google LLC
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import re
> +
> +from avocado_qemu import LinuxTest
> +from avocado_qemu import Test
> +
> +VNET_HUB_HEADER = 'Hub -1 (vnet):'
> +NEIGHBOR_HEADER_REGEX = '^ *Table *MacAddr *IP Address$'
> +
> +def trim(text):
> +    return " ".join(text.split())
> +
> +def hmc(test, cmd):
> +    return test.vm.command('human-monitor-command', command_line=cmd)
> +
> +def get_neighbors(test):
> +    output = hmc(test, 'info neighbors').splitlines()
> +    if len(output) < 2:
> +        test.fail("Insufficient output from 'info neighbors'")
> +    test.assertEquals(output[0], VNET_HUB_HEADER)
> +    test.assertTrue(re.fullmatch(NEIGHBOR_HEADER_REGEX, output[1]))
> +    return output[2:]
> +
> +class InfoNeighborsNone(Test):
> +
> +    def test_no_neighbors(self):
> +        self.vm.add_args('-nodefaults',
> +                         '-netdev', 'user,id=vnet',
> +                         '-device', 'virtio-net,netdev=vnet')
> +        self.vm.launch()
> +        neighbors = get_neighbors(self)
> +        self.assertEquals(len(neighbors), 0)
> +
> +class InfoNeighbors(LinuxTest):
> +
> +    def test_neighbors(self):
> +        """
> +        :avocado: tags=arch:x86_64
> +        :avocado: tags=machine:pc
> +        :avocado: tags=accel:kvm
> +        """
> +        self.require_accelerator('kvm')
> +        self.vm.add_args("-accel", "kvm")
> +        self.vm.add_args('-nographic',
> +                         '-m', '1024')
> +        self.launch_and_wait()
> +
> +        # Ensure there's some packets to the guest and back.
> +        self.ssh_command('pwd')
> +
> +        # We should now be aware of the guest as a neighbor.
> +        expected_ipv4_neighbor = 'ARP 52:54:00:12:34:56 10.0.2.15'
> +        # The default ipv6 net is fec0. Both fe80 and fec0 can appear.
> +        expected_ipv6_neighbors = [
> +            'NDP 52:54:00:12:34:56 fe80::5054:ff:fe12:3456',
> +            'NDP 52:54:00:12:34:56 fec0::5054:ff:fe12:3456'
> +        ]
> +        neighbors = get_neighbors(self)
> +        self.assertTrue(len(neighbors) >= 2 and len(neighbors) <= 3)
> +        # IPv4 is output first.
> +        self.assertEquals(trim(neighbors[0]), expected_ipv4_neighbor)
> +        for neighbor in neighbors[1:]:
> +            self.assertTrue(trim(neighbor) in expected_ipv6_neighbors)
> --
> 2.33.0.153.gba50c8fa24-goog
>
>
>