On Thu, Jan 16, 2025 at 1:27 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2025/01/16 10:17, Jason Wang wrote:
> > On Wed, Jan 15, 2025 at 1:17 PM Akihiko Odaki <akihiko.od...@daynix.com>
> > wrote:
> >>
> >> On 2025/01/13 11:59, Jason Wang wrote:
> >>> On Sat, Jan 11, 2025 at 1:43 PM Akihiko Odaki <akihiko.od...@daynix.com>
> >>> wrote:
> >>>>
> >>>> Hi Jason,
> >>>>
> >>>> Can you check this patch again?
> >>>
> >>> I would like to have this if
> >>>
> >>> 1) it would be used by libvirt.
> >>>
> >>> or
> >>>
> >>> 2) there's no other way to do this
> >>
> >> I need this to make QEMU work with macvtap on mkosi, and this patch is
> >> an effective way to accomplish the goal.
> >
> > I'm not sure how to define "effective" here.
>
> I just meant it requires me writing less code.
>
> >
> >>
> >> Requiring to pass a file descriptor is simply less convenient. Most (if
> >> not all) aspects of QEMU can be configured without file descriptors; I
> >> don't think there is a reason to make tap exceptional.
> >
> > TUNSETIFF requires CAP_NET_ADMIN and qemu doesn't want to run with
> > privilege, so fd is prefered in the case of tuntap.
> >
> > For macvtap,ipvtap, though open, doesn't require any privilege.
> > Passing fd via SCM_RIGHTS is still preferable as it eases the
> > interaction with security facilities (for example, you may want to
> > whitelist /dev/tapX for Qemu to access etc).
>
> That is true for almost any kind of files, and QEMU provides options to
> specify files with file descriptor for this reason. However, it also
> provides alternative options to specify files with e.g., path for
> convenience.
>
> This patch does not add a entirely-new complex, high-level feature. It
> only pushes the macvtap/ipvtap support to the same level with tuntap and
> other features interacting with files.
Fair enough. I've queued this.
Thanks
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>> Thanks
> >>>
> >>>>
> >>>> 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.od...@daynix.com> wrote:
> >>>>>>>
> >>>>>>> On 2024/10/09 16:41, Jason Wang wrote:
> >>>>>>>> On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki
> >>>>>>>> <akihiko.od...@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.od...@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
> >>>>
> >>>
> >>
> >
>
