Re: [PATCH] tap-linux: Open ipvtap and macvtap

[Akihiko Odaki]

Hi Jason,

Can you check this patch again?

Regards,
Akihiko Odaki

On 2024/10/22 13:59, Akihiko Odaki wrote:
> On 2024/10/18 17:10, Jason Wang wrote:
> > On Sat, Oct 12, 2024 at 5:05 PM Akihiko Odaki 
> > <akihiko.odaki@daynix.com> wrote:
> > > On 2024/10/09 16:41, Jason Wang wrote:
> > > > On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki 
> > > > <akihiko.odaki@daynix.com> wrote:
> > > > > ipvtap and macvtap create a file for each interface unlike tuntap, 
> > > > > which
> > > > > creates one file shared by all interfaces. Try to open a file 
> > > > > dedicated
> > > > > to the interface first for ipvtap and macvtap.
> > > >
> > > > Management layers usually pass these fds via SCM_RIGHTS. Is this for
> > > > testing purposes? (Note that we can use something like -netdev
> > > > tap,fd=10 10<>/dev/tap0).
> > >
> > > I used this for testing.
> >
> > Anything that prevents you from using fd redirection? If not
> > management interest and we had already had a way for testing, I tend
> > to not introduce new code as it may bring bugs.
>
> I don't know what ifindex the macvtap device has so it's easier to use 
> if QEMU can automatically figure out the it.
>
> > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > > ---
> > > > >    net/tap-linux.c | 17 ++++++++++++++---
> > > > >    1 file changed, 14 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/net/tap-linux.c b/net/tap-linux.c
> > > > > index 1226d5fda2d9..22ec2f45d2b7 100644
> > > > > --- a/net/tap-linux.c
> > > > > +++ b/net/tap-linux.c
> > > > > @@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int 
> > > > > *vnet_hdr,
> > > > >        int len = sizeof(struct virtio_net_hdr);
> > > > >        unsigned int features;
> > > > >
> > > > > -    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> > > > > +
> > > > > +    ret = if_nametoindex(ifname);
> > > > > +    if (ret) {
> > > > > +        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
> > > > > +        fd = open(file, O_RDWR);
> > > > > +    } else {
> > > > > +        fd = -1;
> > > > > +    }
> > > > > +
> > > > >        if (fd < 0) {
> > > > > -        error_setg_errno(errp, errno, "could not open %s", 
> > > > > PATH_NET_TUN);
> > > > > -        return -1;
> > > > > +        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> > > >
> > > > Any reason tuntap were tried after the macvtap/ipvtap?
> > >
> > > If we try tuntap first, we will know that it is not tuntap when calling
> > > TUNSETIFF. We will need to call TUNGETFEATURES and TUNSETVNETHDRSZ again
> > > in such a case because they precede TUNSETIFF. Calling them twice is
> > > troublesome.
> >
> > I may miss something, we are only at the phase of open() not TUNSETIFF?
>
> We can tell if it is macvtap/ipvtap just by trying opening the device 
> file. That is not possible with tuntap because tuntap uses /dev/net/tun, 
> a device file common for all tuntap interfaces and its presence does not 
> tell if the interface is tuntap.
>
> > > This is also consistent with libvirt. libvirt first checks if
> > > g_strdup_printf("/dev/tap%d", ifindex) exists, and falls back to tuntap
> > > otherwise.
> >
> > This is not what I understand from how layered products work. Libvirt
> > should align with Qemu for low level things like TAP, not the reverse.
>
> This change is intended for the use case where libvirt is not in use. In 
> particular, I use mkosi, which is not a full fledged layering mechanism.
>
> Regards,
> Akihiko Odaki