Skip to content

Conversation

@Mygod
Copy link
Contributor

@Mygod Mygod commented May 8, 2020

I am not going to pretend I know what I am doing with these code.

Elon Musk

@Mygod
Copy link
Contributor Author

Mygod commented May 8, 2020

@madeye @zonyitoo Please review it more carefully I really do not know what I am doing.

@Mygod Mygod changed the title Refactoring for configurable upstreams Refactoring for configurable DNS upstreams May 8, 2020
@madeye
Copy link
Contributor

madeye commented May 9, 2020

Can we also have this? madeye@055b4f6

@Mygod Mygod force-pushed the dns-refine-patch-2 branch from f7c5b08 to 2f1d83e Compare May 9, 2020 01:19
@Mygod Mygod force-pushed the dns-refine-patch-2 branch from c8dac67 to 47bb6ae Compare May 9, 2020 01:24
@Mygod Mygod changed the title Refactoring for configurable DNS upstreams Refactoring for configurable DNS upstreams & some other refinements May 9, 2020
@Mygod
Copy link
Contributor Author

Mygod commented May 9, 2020

With these changes, Android client should behave reasonably with any reasonable ACL file.

@madeye
Copy link
Contributor

madeye commented May 10, 2020

49966d9 seems breaking everything...

@Mygod
Copy link
Contributor Author

Mygod commented May 10, 2020

@madeye Are you seeing 'async fn' resumed after completion? Or something else?

@Mygod
Copy link
Contributor Author

Mygod commented May 10, 2020

Try 13bcc5d.

@Mygod
Copy link
Contributor Author

Mygod commented May 10, 2020

@madeye @zonyitoo Done for today. Want to review these first?

@madeye
Copy link
Contributor

madeye commented May 10, 2020

13bcc5d works, but then 9712a97 breaks ACL again.

To reproduce the issue, please use bypass China route.

@madeye
Copy link
Contributor

madeye commented May 10, 2020

d1a3ac6 should fix the issue.

Copy link
Contributor

@madeye madeye left a comment

Choose a reason for hiding this comment

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

LGTM

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 10, 2020

I am getting more and more confused about what these code are working for. LoL.

Nevermind. Since @madeye has approved, then please @Mygod merge master and fix conflicts.

@Mygod
Copy link
Contributor Author

Mygod commented May 10, 2020

@zonyitoo In that case feel free to comment on where the code doesn't make sense and I can try to explain... Also maybe just merge this instead of rebasing it? There are too many commits now and I don't want to squash everything.

@zonyitoo zonyitoo merged commit 3c04079 into shadowsocks:master May 10, 2020
@zonyitoo
Copy link
Collaborator

I saw in ACL, resolving DomainName to IPs and then check whether IPs are in configured lists, have been removed. Why?

@zonyitoo
Copy link
Collaborator

zonyitoo commented May 10, 2020

Ah, my fault. I found that just now.

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.

3 participants