| Message ID | 20241008-macvtap-v1-1-2032caa25b6d@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tap-linux: Open ipvtap and macvtap | expand |
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). > 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 (fd < 0) { > + error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN); > + return -1; > + } > } > memset(&ifr, 0, sizeof(ifr)); > ifr.ifr_flags = IFF_TAP | IFF_NO_PI; > > --- > base-commit: 31669121a01a14732f57c49400bc239cf9fd505f > change-id: 20241008-macvtap-b152e5abb457 > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com> Thanks >
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. > >> 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. This is also consistent with libvirt. libvirt first checks if g_strdup_printf("/dev/tap%d", ifindex) exists, and falls back to tuntap otherwise. Regards, Akihiko Odaki > >> + if (fd < 0) { >> + error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN); >> + return -1; >> + } >> } >> memset(&ifr, 0, sizeof(ifr)); >> ifr.ifr_flags = IFF_TAP | IFF_NO_PI; >> >> --- >> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f >> change-id: 20241008-macvtap-b152e5abb457 >> >> Best regards, >> -- >> Akihiko Odaki <akihiko.odaki@daynix.com> > > Thanks > >> >
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. > > > > >> 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? > > 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. Thanks > > Regards, > Akihiko Odaki > > > > >> + if (fd < 0) { > >> + error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN); > >> + return -1; > >> + } > >> } > >> memset(&ifr, 0, sizeof(ifr)); > >> ifr.ifr_flags = IFF_TAP | IFF_NO_PI; > >> > >> --- > >> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f > >> change-id: 20241008-macvtap-b152e5abb457 > >> > >> Best regards, > >> -- > >> Akihiko Odaki <akihiko.odaki@daynix.com> > > > > Thanks > > > >> > > >
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
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)); + if (fd < 0) { + error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN); + return -1; + } } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
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. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- net/tap-linux.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) --- base-commit: 31669121a01a14732f57c49400bc239cf9fd505f change-id: 20241008-macvtap-b152e5abb457 Best regards,