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

Combine wslbridge2 and hvpty #11

Closed
Biswa96 opened this issue Oct 2, 2019 · 30 comments
Closed

Combine wslbridge2 and hvpty #11

Biswa96 opened this issue Oct 2, 2019 · 30 comments
Labels
discussion 💬 General discussion or Q&A

Comments

@Biswa96
Copy link
Owner

Biswa96 commented Oct 2, 2019

  • Topic: Combine wslbridge2 (for WSL1) and hvpty (for WSL2) into one binary both for backend and frontend. They share most of the option processing and how wsl.exe executes the backend. The difference is the socket in use -- wslbridge2 uses AF_INET (wrapped by cygwin) but hvpty uses raw AF_HYPERV from winsock. Also the multi-threading execution is different between them.

  • Pros:

    • frontend (PE binary) and backend (ELF binary) will detect WSL version V1 or V2 internally, so upstream programs do not have to do extra works.
    • Mitigate any confusion among users about which one to use.
  • Cons:

  • To do:

    • Fix any remaining issues first. Top priority 🚨
    • The two backends can easily combined into one (v0.4), I'll do it after fixing the current issues.
    • Next combine the frontend (v0.5). But there are many hurdles for it. See next section.
    • Try not to create any regression.
  • Problems:

  • Conclusion: Throw your suggestion or idea on how to solve this. Any suggestions are greatly appreciated 🤗

@mintty
Copy link
Contributor

mintty commented Oct 2, 2019

I would not worry about startup performance, certainly not due to neglectable software details.
To detect V1/V2, you could also check the registry; this is what mintty currently does.
But I agree it's a nuisance that neither way is currently specified.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 2, 2019

I think most of wslbridge users uses wslbridge for launching a login shell instead of running hundreds of commands under Cygwin, so I don't think startup performance is critical.
Also, I used registry checking for finding distros, there is an explicit entry for WSL version of each distro.
Another way is executing wsl -l -v to acquire distro info. This should be a documented way.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 2, 2019

Also, If it is possible to let the backend tell the frontend what the WSL version is?
For example, the frontend bind on both AF_INET and AF_HYPERV as initial port, and then the backend decide which one to connect.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 2, 2019

The idea is good but the such method is different between them. I'll think about it. First I'm focusing on remaining issues.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 3, 2019

Also, is it possible to use execvp instead of CreateProcessW in frontend if we finally make the large refactor? Thus we can make an argument list instead of building a command, because building a command with a series of string operations maybe confusing and it is really difficult to debug.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 3, 2019

execvp is in cygwin world. It doesn't have the configuring option StartupInfoExW and other flags in CreateProcessW.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 3, 2019

There are some issues with IP addresses in WSL2. I want to add those IP addesses in WSL_HOST_IP and WSL_GUEST_IP environment variable (once suggested by @therealkenc). Which commandline option do you guys prefer for hvpty? Code is somewhat ready.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 4, 2019

There are some issues with IP addresses in WSL2. I want to add those IP addesses in WSL_HOST_IP and WSL_GUEST_IP environment variable (once suggested by @therealkenc). Which commandline option do you guys prefer for hvpty? Code is somewhat ready.

Do you mean also use AF_INET interface in the hvpty? How do you get those ip addresses? I also find the ip addresses in WSL2 changes each time you reboot the windows.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 4, 2019

@dxhisboy code added. Can you test for any bug? There would be a release in 2 days.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 5, 2019

Sorry, I'm travelling these days. Maybe I can have a try after Oct. 8.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 18, 2019

backends are combined. Suggestions needed for frontends.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 21, 2019

Great work!
I browsed some code in hvsocket, I think you can use hvpty as an entrance, and then if it failed to get VMID, then try listenning on AF_INET.
I think we can make an AF_INET implementation of HyperVSocket, inheriting from same parent class, when we find a distro is WSL1, then use AF_INET implementation.
Maybe I can try doing so.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

Thank you for the reply. Here are my thoughts:

  1. Convert LocalSock class to use Windows socket instead of cygwin.
  2. Then combine two classes normally (without any inheritence, virtual table).
  3. For WSL version detection, I choose WslGetDistributionConfiguration() flag greater than 7.

@dxhisboy
Copy link
Contributor

I made a little experiment and I now agree to start from wslbridge2's LocalSock.
Our implementation does support a listen of hvsocket on a "random" port now thus we can merge the startup logic.
I also agree to use windows socket, since it will provide the AF_HYPERV support.
Maybe passing VmId == NULL to open a INET socket is OK.
I think detecting WSL version can be done with established code or interface in GetVmId.cpp, while the implementation can be changed as you want.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

GetVmId.cpp uses undocumented COM method of ILxssUserSession interface which is way different in older Windows 10 versions**. So, it will be better to use any "documented way" for WSL1 and WslGetDistributionConfiguration() is in that way.

** See WslReverse for the history lesson :)

@dxhisboy
Copy link
Contributor

Also, a little question, why do you use LoadLibraryExW for HyperVSocket? Is it to prevent sys/sock functions from overriding winsock functions? If so, I think the h_Module should be a singleton, or do not call WSACleanup to avoid the risk of recycling any HyperVSocket will call WSACleanup.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

Also, a little question, why do you use LoadLibraryExW for HyperVSocket?

Welcome to cygwin tricks :) https://cygwin.com/ml/cygwin/2019-10/msg00114.html

I think the h_Module should be a singleton

Is it necessary the class is instantiated once?

@dxhisboy
Copy link
Contributor

Is it necessary the class is instantiated once?

I think pointing a static pointer initially to nullptr, and a static function of getInstance is corresponding to check is this pointer initialized.

@mintty
Copy link
Contributor

mintty commented Oct 21, 2019

WslGetDistributionConfiguration() flag greater than 7.

I'd rather test for bit 3 (0x08) as higher bits might be used for other purposes in future.

I think the h_Module should be a singleton

Is it necessary the class is instantiated once?

I'd suggest not to over-engineer this fine but small tool. We're not doing object-oriented system design here.

@dxhisboy
Copy link
Contributor

I'd suggest not to over-engineer this fine but small tool. We're not doing object-oriented system design here.

I think so, but, how to know, we should do WSACleanUp? Or, as a small tool, we can never clean up.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 21, 2019

I briefly merged the frontend on wslbridge2 at https://github.com/dxhisboy/wslbridge2.
Not quite sure will it work well.
And still a number of ugly codes.
Also, make hvpty first before making wslbridge2 because dependency issue, and I'm now doing some fixes.

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

I'd rather test for bit 3 (0x08) as higher bits might be used for other purposes in future.

@mintty The idea is good. If higher bits is used in future I will change it. I speculate that MS devs are moving from using registry. If you follow the current WSL developments you can see the default uid and kernel commandline are now configured in %UserProfile%\.wslconfig file.

I'd suggest not to over-engineer this fine but small tool

Any programming hacks are greatly appreciated until it does not affect performance.

Biswa96 added a commit that referenced this issue Oct 21, 2019
Biswa96 added a commit that referenced this issue Oct 21, 2019
@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

Combination completed. Need hardcore testing.

@mintty
Copy link
Contributor

mintty commented Oct 21, 2019

Makefile still wants hvpty which is gone.

Biswa96 added a commit that referenced this issue Oct 21, 2019
@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 21, 2019

From @dxhisboy's query:

why do you use LoadLibraryExW for HyperVSocket?

The LoadLibraryExW() may be removed. GCC in cygwin successfully links with ws2_32.dll (with -lws2_32 option). But the same functions are exported from cygwin1.dll. I don't know if linker will always choose ws2_32.dll as first preference.

Another alternative way is to use WSA- prefixed functions, for example WSASocket() instead of socket() and others. But getsockname() and setsockopt() do not have those cousins.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 22, 2019

I don't know if linker will always choose ws2_32.dll as first preference.

Not sure. Maybe you need a ldscript to ensure this.

By default, ws2_32.dll overrided cygwin32. You can add -Wl,-y,socket when linking to see where is a symbol defined.

dxhis@xduan-ux410uqk ~
$ cat testsocket.c
int main(){
  int x = socket(0, 0, 0);
}

dxhis@xduan-ux410uqk ~
$ gcc testsocket.c -lws2_32 -Wl,-y,socket
testsocket.c: In function ‘main’:
testsocket.c:2:11: warning: implicit declaration of function ‘socket’ [-Wimplicit-function-declaration]
   int x = socket(0, 0, 0);
           ^~~~~~
/tmp/cclkJl2t.o: reference to socket
/usr/lib/w32api/libws2_32.a(dqwms00181.o): definition of socket

dxhis@xduan-ux410uqk ~
$ gcc testsocket.c  -Wl,-y,socket
testsocket.c: In function ‘main’:
testsocket.c:2:11: warning: implicit declaration of function ‘socket’ [-Wimplicit-function-declaration]
   int x = socket(0, 0, 0);
           ^~~~~~
/tmp/ccGyMOtN.o: reference to socket
/usr/lib/gcc/x86_64-pc-cygwin/7.4.0/../../../../lib/libcygwin.a(t-d001364.o): definition of socket

dxhis@xduan-ux410uqk ~
$

When I add -v option, the collect2 cmdline shows -lws2_32 -y socket -lgcc_s -lgcc -lcygwin -ladvapi32 -lshell32 -luser32 -lkernel32 -lgcc_s -lgcc. If gcc always put user provided ld parameter before default library, this will be ensured. According to their logic, when ld found socket in ws2_32, it will give up searching socket in other libraries.
I wanted to use objdump to find more details, but seems this is not in the linux world.
I used to use a very tricky linker named sw5ld, during the experience I can tell you a very ugly method to ensure this: extract dqwms00181.o in /usr/lib/w32api/libws2_32.a, and pass it as an object when linking, then ld processes it, a function in object file have higher priority than any library functions, then it is guaranteed to use win32's socket function. Too ugly to describe this

@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 22, 2019

when ld found socket in ws2_32, it will give up searching socket in other libraries.

This is what man ld says. But does cygwin follow this rule always? Do other linkers follow this? The -lws2_32 is just for experimenting, I will not use it as in future cygwin may change the rule, who knows 🤷

@dxhisboy
Copy link
Contributor

But does cygwin follow this rule always?
I'm not sure, this is not in the linux world.
But, theoretically, what if we firstly use ld -r to link a object with the libws2_32.a before the real linking? It will result in a .o file which contains the definition of socket:

dxhis@xduan-ux410uqk ~
$ gcc testsocket.c -c
testsocket.c: In function ‘main’:
testsocket.c:2:11: warning: implicit declaration of function ‘socket’ [-Wimplicit-function-declaration]
   int x = socket(0, 0, 0);
           ^~~~~~

dxhis@xduan-ux410uqk ~
$ ld -r testsocket.o /usr/lib/w32api/libws2_32.a -o testsocket_ws2_32.o

dxhis@xduan-ux410uqk ~
$ gcc testsocket_ws2_32.o  -Wl,-y,socket
testsocket_ws2_32.o: definition of socket

I do not think ld will search system libs when doing a relocatable link. Also, I do think ld will process object functions earlier than library function. Thus, this will result in using ws2_32's socket with a somehow guaranteed procedure.

@dxhisboy
Copy link
Contributor

dxhisboy commented Oct 22, 2019

Also, it is not required to pass the full path of libws2_32.a. Using -lws2_32 with -r can also handle this.

dxhis@xduan-ux410uqk ~
$ ld -r testsocket.o -lws2_32 -o testsocket_ws2_32.o

dxhis@xduan-ux410uqk ~
$ gcc -Wl,-y,socket testsocket_ws2_32.o
testsocket_ws2_32.o: definition of socket

Maybe just write missing WSA prefixed functions in one file, and relocatablely link it with ws2_32, then use this .o file in the final linking procedure may do the hack.

Biswa96 added a commit that referenced this issue Oct 22, 2019
@Biswa96
Copy link
Owner Author

Biswa96 commented Oct 22, 2019

Thank you all.

@Biswa96 Biswa96 closed this as completed Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 General discussion or Q&A
Projects
None yet
Development

No branches or pull requests

3 participants