Re: [PATCH v2 2/2] dts: add virtio forwarding test suite

[Dean Marx <dmarx@iol.unh.edu>]

On Tue, Oct 21, 2025 at 11:13 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> On Fri, Oct 03, 2025 at 03:27:16PM +0000, Dean Marx wrote:
> > Add test suite covering virtio-user and vhost
> > server/client forwarding scenarios with
> > testpmd packet validation.
> >
> > Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
<snip>
> > +
> > +virtio_fwd Test Suite
> > +===========================
> this is fine, but ideally should be aligned with the heading.

I'll change this, I just noticed I put it in the wrong directory too
so thanks to you and Patrick for pointing that out

<snip>
> > +    class vdevs:
> > +        """Class containing virtio-user and vhost-user virtual devices."""
> is it actually vhost-user? or just vhost?

It's vhost-user since it's using a unix socket path (/tmp/vhost-net),
whereas a vhost version would use a kernel network interface like tap0
or something similar.

> > +
> > +        virtio_user = VirtualDevice(
> > +            "net_virtio_user0,mac=00:01:02:03:04:05,path=/tmp/vhost-net,server=1"
> > +        )
> > +        vhost_user = VirtualDevice("eth_vhost0,iface=/tmp/vhost-net,client=1")
>
> How come you put these properties under some internal classes? Not sure
> how this helps. The vdevs class should technically be capitalised as
> well. Either way if these are just class variables, they should be made
> as such. Doing the following:
>
>   parse_rx_packets: ClassVar[ParserFn] = TextParser...
>   parse_tx_packets: ...
>   virtio_user_vdev: ClassVar[VirtualDevice] = ...
>   vhost_user_vdev: ...
>
> is perfectly fine and acceptable.
>
> I've changed the naming of the parser functions, because you are
> effectively treating these as functions, then they should be named as
> such.

Got it, I'll change this

<snip>
> > +
> > +            rx_packets = self.ForwardingParsers.rx_packets["TextParser_fn"](forwarding_stats) or 0
> > +            tx_packets = self.ForwardingParsers.tx_packets["TextParser_fn"](forwarding_stats) or 0
>
> So I understand you are basically reusing the functionality of the
> TextParser functions here. I guess it's not a bad shout but I would not
> expose the internals which are only meant to be used within the
> TextParser class for handling with generic dataclass metadata.
>
> Ideally we want this:
>
>   rx_packets = self.parse_rx_packets(forwarding_stats) or 0
>
> You can implement __call__ in ParserFn to make this happen. In the best
> scenario we'd want the return type to be correct as well instead of just
> Any. If you want to, you can play around and implement a Generic[T] in
> ParserFn to replace Any with T. This will probably require you to make
> changes elsewhere though. It may get tricky easily, so it's not really
> mandatory.

Hmm okay, I'll try to write a workaround for this without
overcomplicating things

<snip>
> > +        with TestPmd(
> > +            prefix="virtio",
> > +            vdevs=[VirtualDevice("virtio_user0,path=/dev/vhost-net,queues=1,queue_size=1024")],
> > +        ) as virtio:
> > +            self.sut_node.main_session.send_command("ip link set dev tap0 up", privileged=True)
>
> This should have been implemented as part of main_session. If it's not,
> it should. Shouldn't call send_command directly like this.

I'll add this to the next version

>
> > +            with TestPmd(
> > +                prefix="vhost", no_pci=True, vdevs=[VirtualDevice("net_af_packet0,iface=tap0")]
> > +            ) as vhost:
> > +                virtio.set_forward_mode(SimpleForwardingModes.mac)
> > +                vhost.set_forward_mode(SimpleForwardingModes.mac)
> > +                vhost.start()
> > +                virtio.start()
> > +
> > +                packet = Ether() / IP()
> > +                packets = [packet] * 100
> > +                captured_packets = self.send_packets_and_capture(packets)
> > +
> > +                self.verify(
> > +                    len(captured_packets) >= 100, "Sent packets not received on physical interface."
> > +                )
> > --
> > 2.51.0
> >