Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

Use alternative split char #205

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

Conversation

ratcashdev
Copy link

to solve #204

@BLumia
Copy link
Contributor

BLumia commented Nov 13, 2019

Good catch but it seems it will make the existed config file no longer works, maybe do a character escape like \@ and unsecape it when using them instead of simply calling .split("@"); can be a better approach?

@ratcashdev
Copy link
Author

@BLumia not sure. Wouldn't an escaped \@ still count as a @ when splitting? I am afraid it will.

As for existing config files... you're right. I have added backward compatibility in the latest commit.

Copy link
Contributor

@BLumia BLumia left a comment

Choose a reason for hiding this comment

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

Looks okay for me, @justforlxz any suggestions?

@BLumia not sure. Wouldn't an escaped \@ still count as a @ when splitting? I am afraid it will.

In that case we cannot just simply do a split :)

As for existing config files... you're right. I have added backward compatibility in the latest commit.

Good, I'll check if | can be yet another usable character or not, then I think we can merge it :)

Copy link
Contributor

@BLumia BLumia left a comment

Choose a reason for hiding this comment

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

I suggest use : as the separator if you don't want to use escaped character and do escape/unescape while read/write. Since both @ and | are valid to use in username. Try sudo useradd 'a|a' and sudo useradd 'a@a' and see the result.

Since /etc/passwd use : as separator, and sudo useradd 'a:a' will also give us a useradd: invalid user name 'a:a' output, I guess it can be safe to use : as the separator.

Copy link
Contributor

@BLumia BLumia left a comment

Choose a reason for hiding this comment

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

Ah really sorry I forget IPv6 address can have :. Then I guess maybe we cannot continue use a simply split().

I guess the best way is continue using this format but only treat last two @ as valid separator or do string escape and unescape manually. What do you think?

@ratcashdev
Copy link
Author

I think having a | is far less likely and problematic than anything else. Plus I still don't see how escaping would help. Care to explain?

@BLumia
Copy link
Contributor

BLumia commented Nov 20, 2019

I think having a | is far less likely and problematic than anything else.

We're glad you found the @ split char issue. But since we now know there can be issue, we should avoid any possible case which can cause the same issue like using | or :. So IMO "far less likely and problematic than anything else" is also not acceptable.

Plus I still don't see how escaping would help. Care to explain?

A escaped string can be username\@[email protected]@port. Then we need to implement a simple split() function to do split only when it found a @ instead of \@. But it can be a little complex if we need also treat the escaped character\\.

A much simpler way to implement (instead of using the escaped/unescaped approach) is just treat last two @ as valid split char as I already said before, so there will be no problem with the original config file format, and easy to implement.

In string.split()there is a max_tokens argument to use. Since we need the last two @ as valid ones instead of first one, we can do a string.reverse() first before we call split(). Then after the split() call, just reverse() the splited strings back will be okay.

// pseudo code, *not* real vala code.
// reverse the original string, split.
server_infos = server_info.reverse().split("@", 3);
// reverse the result list. strings in the list still need to be reverse(). 
server_infos.reverse();
// reverse the strings inside the list.
server_infos.foreach ((entry) => {
	entry.reverse();
});

Or other ways can also works like just simply split with @ and string join all entries without the last two string. Or maybe you have any better approach which also works :)

@ratcashdev
Copy link
Author

Possibly. Or BASE64 encode username/host and then we can use split with the current char (@).

@magynhard
Copy link

@BLumia i like your suggestion, it is backward compatible and also solves the original problem as well, without users needing to know, that they have to use special characters in that case. I think it's the most intuitve solutation.

@ratcashdev BASE64 is an approach, but will not work with old configuration files - you may introduce some patterns so ensure if the file is encoded or not and to do that at the next time you save the file. So the solution from @BLumia seems be an quick win.

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

Successfully merging this pull request may close these issues.

3 participants