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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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