Since I can’t edit it anymore, string_split(connect_host, “:”) should be changed to string_split(connect_host, “|”) to support IPv6 addresses.
Hiiii 
Just following up from this discussion:
Would definitely appreciate any thoughts and feedback after you’ve read through the comments, @anon72693988. I don’t see any downside about adding the flexibility of being backwards compatible.
Hey, you should go through this thread’s discussion to understand why it was detached from the config append behavior, which --port still uses.
I would suggest that if you want to use --port for connect instead of writing it in the same argument, refactor the command-line argument in the same fashion, storing the command-line port outside of the config and setting it to 0 after its first use. Remember that --port is also used by --H/–host.
Thank you for the response! I have read thru the thread, and I understand why you are making the changes that you are making. Thank you for your contribution and support!
My PR does not undo anything you’ve done. But the port parameter should be able to be used whether hosting or joining for backwards compatibility. Your commit doesn’t allow this currently. Your new format with a pipe will be used first whenever it’s detected, but if it’s not then my PR allows it to fall back to providing the port separately. There’s really no downside that I can think of, but I’m open to discuss ofc.
Side note: I replied to this via emai but it never showed up, so I apologize if this eventually shows up twice haha
The problem is that since --port is still using the old append config behavior, it will fall into the same undefined behavior as the old --connect did when used together with --libretro.
Let’s say we do this “–connect 192.168.0.1 --port 1111 --libretro cores/fbneo_libretro.dll arcade/ssriders.zip” and have netplay_ip_port set to 55435 in our config, what will happen is that the port used to connect to 192.168.0.1 will be 55435 instead of the desired port 1111.
Either way, I would recommend you to wait a bit before modifying that part of the code. My current work on a new working MITM system will include an optional third parameter so that --connect can support connecting to hosts using MITM by passing its base64 session id via this third parameter.
Once again - thanks for your efforts and responses!
I must be overlooking something still, because my change will only pull the port from the command line or config if it’s blank (wasn’t passed with a pipe). Is there another piece of code somewhere which is overriding “settings->uints.netplay_port” when set from both the command line and the config? If so, shouldn’t a quick conditional prior to setting it resolve that concern?
Definitely awesome to hear there is more to come for MITM. But in it’s current state, the latest version of RA is not backwards compatible, which WILL break many integrated systems. Happy to assist with the final solution in any way I’m able in order to make sure we can provide enhancements while maintaining compatibility.
If the decision is made to not be backwards compatible, it’s going to be a very important thing to communicate to the public.
Likely because when using --connect with the --libretro option causes the content to be loaded (and thus netplay to be initialized) before config appends are applied; --port append will be ignored and the one in the main config will be used instead.
I would say 90% or more of the usage of connect would also come with --libretro (otherwise you would just launch RetroArch, forcing the user to manually load core and content), the previous behavior, which is still around for --port has undefined behavior; it will use --port when --libretro is not present and netplay_ip_port from main config when --libretro is present.
Like I’ve mentioned previously, if you want --port to work with --connect, refactor it to store its value into a variable, just like it’s done for “connect_host”. But do wait until the new functionality is implemented for MITM since we will be editing the same code.
I’ve added support for optional parameters via my hostname decode function.
bool netplay_decode_hostname(const char *hostname,
char *address, unsigned *port, char *session, size_t len)
{
struct string_list *hostname_data;
if (string_is_empty(hostname))
return false;
hostname_data = string_split(hostname, "|");
if (!hostname_data)
return false;
if (hostname_data->size >= 1 &&
!string_is_empty(hostname_data->elems[0].data))
strlcpy(address, hostname_data->elems[0].data, len);
if (hostname_data->size >= 2 &&
!string_is_empty(hostname_data->elems[1].data))
{
unsigned tmp_port = strtoul(hostname_data->elems[1].data, NULL, 10);
if (tmp_port && tmp_port <= 65535)
*port = tmp_port;
}
if (hostname_data->size >= 3 &&
!string_is_empty(hostname_data->elems[2].data))
strlcpy(session, hostname_data->elems[2].data, len);
string_list_free(hostname_data);
return true;
}
And this is how it looks on the event:
tmp_netplay_server[0] = '\0';
tmp_netplay_session[0] = '\0';
if (netplay_decode_hostname(p_rarch->connect_host,
tmp_netplay_server, &netplay_port, tmp_netplay_session,
sizeof(tmp_netplay_server)))
{
netplay_server = tmp_netplay_server;
netplay_session = tmp_netplay_session;
}
if (p_rarch->connect_host)
{
free(p_rarch->connect_host);
p_rarch->connect_host = NULL;
}
if (string_is_empty(netplay_server))
netplay_server = settings->paths.netplay_server;
if (!netplay_port)
netplay_port = settings->uints.netplay_port;
As such, your PR is no longer required, although this is intended for the port set in the config, still causing the previously described issue when used with --port. If you want to fix the behavior of --port with --libretro, I would suggest for you to send another PR once this new code is merged into the master branch.
1 Like
Awesome, thanks again! Glad you separated that server and port conditional at the end. At a glance, I’m thinking this change will allow for backwards compatibility, but I’ll test and confirm once it’s merged and resubmit if needed.
1 Like