Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tap update for modern linux versions #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hfmanson
Copy link

This pull request re-enables the tap interface for modern linux versions

  • when tuntap is detected in the configure script ethertap is no longer used and the user can enter tapN as ether device
  • tunconfig (which should be renamed tapconfig IMO) is updated to use ip instead of ifconfig and now creates a tap device which has the current user as owner
  • This pull request contains the temporarily disabled fork

http://backreference.org/2010/03/26/tuntap-interface-tutorial/ has been extremely useful

@karelbilek
Copy link
Contributor

oh, it seems like I am not the only one who did the script fixes. :) well, someone should probably merge this.

@@ -815,7 +815,7 @@ int main(int argc, char **argv)

#if defined(ENABLE_XF86_DGA) && !defined(ENABLE_MON)
// Fork out, so we can return from fullscreen mode when things get ugly
XF86DGAForkApp(DefaultScreen(x_display));
//XF86DGAForkApp(DefaultScreen(x_display));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to the TAP fixes and I don't think should be in this commit.

@asvitkine
Copy link
Collaborator

I left some comments on the change.

@asvitkine
Copy link
Collaborator

I left some comments on the pull request.

On Fri, May 27, 2016 at 8:30 PM, Karel Bílek [email protected]
wrote:

oh, it seems like I am not the only one who did the script fixes. :) well,
someone should probably merge this.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#64 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AABE8N9tRgDm__mbC-l3ctZAParvN8hbks5qF4ysgaJpZM4Dtlqx
.

@dcoshea
Copy link

dcoshea commented Jan 21, 2018

I think this is still quite relevant. I finally understand what this is about after reading it a few times (after seeing how Bochs does it) and I want this patch now, as it means that instead of BasiliskII creating the tap-type interface for me, it will use a pre-created tapN interface. The benefits of this are:

  1. I can create that interface by running e.g. sudo ip tuntap add tap7 mode tap, then run sudo ip ... to configure it, possibly in my startup scripts. My old version of NetworkManager doesn't seem to have a way to create these interfaces, but perhaps newer versions do. Currently, for BasiliskII to create the interface, it either needs to be run as root, or you need to give the binary the cap_net_admin capability (e.g. sudo setcap cap_net_admin+eip PATH-TO-BasiliskII), which is a bit like setuid root but more restrictive. Having the interfaces created outside of BasiliskII is more secure, especially since the suggested solution currently (in the tunconfig script) is to have passwordless sudo access set up for your account.

  2. At a more basic level, BasiliskII currently creates the tap-type interface with a name like tun0 which I found very confusing until I'd read this code and understood the kernel feature and realised that it's really a tap-type interface with the wrong name (note that tap and tun mean two different things).

I will respond to some of the review comments in more detail and perhaps raise some additional concerns. I suppose that after we've discussed the changes, unless @hfmanson appears, I'll need to submit my own PR to replace this one?

@phrxmd
Copy link

phrxmd commented Jan 15, 2019

@dcoshea please do, it looks like @hfmanson is not active here anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants