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

Add remote host/port options for use when forwarding. #2807

Closed
wants to merge 2 commits into from
Closed

Add remote host/port options for use when forwarding. #2807

wants to merge 2 commits into from

Conversation

RipleyTom
Copy link
Contributor

Add 2 new options:
--tunnel-host to set the final remote IP address when using --force-adb-forward.
--tunnel-port to set the final remote port when using --force-adb-forward.

Note that those are different from the forwarding port given through --port(as adb will still forward on that port).
I tried to keep the changes relatively minimal(ie if you don't use the options it will behave as it used to).

I also added a .clang-format file as every C/C++ project should have one.
Another notable change is I forced _WIN32_WINNT and WINVER to 0x0600(from mingw default of 0x0502) through meson to have access to a few more posix inet functions for windows.

Implement #2801.

rom1v pushed a commit that referenced this pull request Nov 19, 2021
PR #2807 <#2807>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v pushed a commit that referenced this pull request Nov 19, 2021
In "adb forward" mode, by default, scrcpy connects to localhost:PORT,
where PORT is the local port passed to "adb forward". This assumes that
the tunnel is established on the local host with a local adb server
(which is the common case).

For advanced usage, add --tunnel-host and --tunnel-port to force the
connection to a different destination.

Fixes #2801 <#2801>
PR #2807 <#2807>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v added a commit that referenced this pull request Nov 19, 2021
Tunnel host and port are only meaningful in "adb forward" mode.

They indicate where the client must connect to communicate with the
server. In "adb reverse" mode, the client _listens_, it does not
_connect_.

Refs #2807 <#2807>
rom1v added a commit that referenced this pull request Nov 19, 2021
@rom1v
Copy link
Collaborator

rom1v commented Nov 19, 2021

Thank you very much. Works as expected 👍

I applied the following changes:

  • extract util method to parse an IPv4, so that cli.c do not have to handle that (and split it to a separate commit)
  • accept --tunnel-port=0, because we should be able to pass the default value ("do not force a port")
  • replace remaining scrcpy_ip and scrcpy_port to tunnel_host and tunnel_port in the code
  • move _WIN32_WINNT and WINVER definitions from meson to source code (see below)
  • document in the manpage (same content as in the --help)
  • document in the README with an example (separate commit)
  • automatically enable --force-adb-forward if tunnel host or port is provided (separate commit)
  • fix minor code style issues

The resulting branch is pr2807. Please review and test :)

Could you please provide a real e-mail address? Your commit author is currently:

RipleyTom <RipleyTom@users.noreply.github.com>

Another notable change is I forced _WIN32_WINNT and WINVER to 0x0600(from mingw default of 0x0502) through meson to have access to a few more posix inet functions for windows.

I did a similar thing recently, but I defined the variable directly in the source code, not via meson. It's more local and does not complexify the build system.

I also added a .clang-format file as every C/C++ project should have one.

This is off-topic. And it would require more work (configure more options in the .clang-format file and make all the source code match the rules).

@RipleyTom
Copy link
Contributor Author

RipleyTom commented Nov 20, 2021

I'm using that email specifically for privacy(see https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-email-preferences/setting-your-commit-email-address).

_WIN32_WINNT and WINVER should be defined at the project level, not in some files and not in others. Windows API version used should be consistent among the whole project.

I don't really care about the .clang-format but it was a terrible experience trying to contribute to the project without one(I just set enough options for it to format the functions correctly).

I like the other changes 👍 .

@rom1v
Copy link
Collaborator

rom1v commented Nov 20, 2021

I'm using that email specifically for privacy(see https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-email-preferences/setting-your-commit-email-address).

OK.

_WIN32_WINNT and WINVER should be defined at the project level, not in some files and not in others. Windows API version used should be consistent among the whole project.

Here is what the GNU documentation says:

You should define these macros by using '#define’ preprocessor directives at the top of your source code files. These directives must come before any #include of a system header file. It is best to make them the very first thing in the file, preceded only by comments. You could also use the ‘-D’ option to GCC, but it’s better if you make the source files indicate their own meaning in a self-contained way.

Is it different for Windows?

I don't really care about the .clang-format but it was a terrible experience trying to contribute to the project without one(I just set enough options for it to format the functions correctly).

What's your editor?

@RipleyTom
Copy link
Contributor Author

RipleyTom commented Nov 20, 2021

_WIN32_WINNT and WINVER should be defined at the project level, not in some files and not in others. Windows API version used should be consistent among the whole project.

Here is what the GNU documentation says:

You should define these macros by using '#define’ preprocessor directives at the top of your source code files. These directives must come before any #include of a system header file. It is best to make them the very first thing in the file, preceded only by comments. You could also use the ‘-D’ option to GCC, but it’s better if you make the source files indicate their own meaning in a self-contained way.

Is it different for Windows?

The goal of _WIN32_WINNT/WINVER is to indicate a minimum API version(and thus windows version) necessary for the program to run, win32 api is backward compatible(sometimes to a fault) so there is no drawback to setting the minimum project wide(unlike the cited page which has an explanation about incompatible standards).
This project is quite small so it's fine if you want to use #define macros each time you want to use a more advanced version of the API but on a bigger project it'd lead to issues with redefinitions if you use different values in headers or the like, just not a good idea.

I don't really care about the .clang-format but it was a terrible experience trying to contribute to the project without one(I just set enough options for it to format the functions correctly).

What's your editor?

You know what's the point of .clang-format? Not having to worry about which editor people are using ;).

rom1v added a commit that referenced this pull request Nov 20, 2021
Refs #2807 <#2807>

Co-authored-by: RipleyTom <RipleyTom@users.noreply.github.com>
@rom1v
Copy link
Collaborator

rom1v commented Nov 20, 2021

Hmm, in fact, the way these feature test macros are currently defined is not very good:

#define _POSIX_C_SOURCE 200809L
#define _XOPEN_SOURCE 700
#define _GNU_SOURCE
#ifdef __APPLE__
# define _DARWIN_C_SOURCE
#endif

In VLC, in fact we define them in config.h (via autootols). But I submitted #2812 with add_global_arguments(). Please review 😉

rom1v added a commit that referenced this pull request Nov 20, 2021
Refs #2807 <#2807>

Co-authored-by: RipleyTom <RipleyTom@users.noreply.github.com>
rom1v added a commit that referenced this pull request Nov 20, 2021
Refs #2807 <#2807>

Co-authored-by: RipleyTom <RipleyTom@users.noreply.github.com>
rom1v added a commit that referenced this pull request Nov 20, 2021
Refs #2807 <#2807>

Co-authored-by: RipleyTom <RipleyTom@users.noreply.github.com>
rom1v pushed a commit that referenced this pull request Nov 21, 2021
PR #2807 <#2807>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v pushed a commit that referenced this pull request Nov 21, 2021
In "adb forward" mode, by default, scrcpy connects to localhost:PORT,
where PORT is the local port passed to "adb forward". This assumes that
the tunnel is established on the local host with a local adb server
(which is the common case).

For advanced usage, add --tunnel-host and --tunnel-port to force the
connection to a different destination.

Fixes #2801 <#2801>
PR #2807 <#2807>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v added a commit that referenced this pull request Nov 21, 2021
Tunnel host and port are only meaningful in "adb forward" mode.

They indicate where the client must connect to communicate with the
server. In "adb reverse" mode, the client _listens_, it does not
_connect_.

Refs #2807 <#2807>
rom1v added a commit that referenced this pull request Nov 21, 2021
@rom1v
Copy link
Collaborator

rom1v commented Nov 21, 2021

Merged into dev.

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.

2 participants