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.

Sign up for GitHub

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

Jump to bottom

Use executable path as plugin path #89

Merged
merged 3 commits into from
Mar 8, 2018
Merged

Use executable path as plugin path #89

merged 3 commits into from
Mar 8, 2018

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Mar 6, 2018 •
edited by faern
Loading

Checklist for a PR:

  • Describe the change in CHANGELOG.md. Only applicable if the change has any impact for a user.

This change is Reviewable

@pronebird
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

            .parent()
            .map(Path::to_path_buf)
            .ok_or(ErrorKind::ExecutableHasNoParentDir.into())

I am a little bit confused about that error. How's it possible that executable has no parent directory?

I can only see that happen if it's running from memory location but then current_exe() would fail anyway.

Thoughts?

Comments from Reviewable

@faern
Copy link
Member

faern commented Mar 6, 2018

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

talpid-core/src/tunnel/mod.rs, line 215 at r1 (raw file):

    fn get_executable_dir() -> Result<PathBuf> {
        let exe_path = env::current_exe().chain_err(|| ErrorKind::ExecutablePathInaccessible)?;

Regarding error handling. I feel it's easy to have an explosion of errors that can virtually never happen. If you look at get_resource_dir() in main.rs do you think the approach we took there has a problem with it? If any error happens along the way we just default to the . path.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

            .parent()
            .map(Path::to_path_buf)
            .ok_or(ErrorKind::ExecutableHasNoParentDir.into())

Strictly according to the types, this error can happen. But can it happen in practice? If the exe_path is a file (it should always be?) then it must have a parent, all files have a parent. The only path that return None on parent() should be / right?

Which means it should be fairly safe to either just unwrap() here, or to fallback to the fallback stated above.

Comments from Reviewable

@faern
Copy link
Member

faern commented Mar 6, 2018

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

I am a little bit confused about that error. How's it possible that executable has no parent directory?

I can only see that happen if it's running from memory location but then current_exe() would fail anyway.

Thoughts?

What does "running from memory location" mean?

Comments from Reviewable

@faern
Copy link
Member

faern commented Mar 6, 2018

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Strictly according to the types, this error can happen. But can it happen in practice? If the exe_path is a file (it should always be?) then it must have a parent, all files have a parent. The only path that return None on parent() should be / right?

Which means it should be fairly safe to either just unwrap() here, or to fallback to the fallback stated above.

Which is why we use pop() instead of parent() over there, since it does not have a failure really, it just stays the same path if it does not have one.

Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

What does "running from memory location" mean?

You can google "launching exe from a memory buffer"

Comments from Reviewable

@pronebird
Copy link
Contributor

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

Comments from Reviewable

@jvff
Copy link
Contributor Author

jvff commented Mar 6, 2018

The Travis CI build failed, but it was due to a compiler bug (rust-lang/rust#48336).

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

talpid-core/src/tunnel/mod.rs, line 215 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Regarding error handling. I feel it's easy to have an explosion of errors that can virtually never happen. If you look at get_resource_dir() in main.rs do you think the approach we took there has a problem with it? If any error happens along the way we just default to the . path.

Agreed. I removed both errors and used the same approach as in get_resource_dir(). Thanks!

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Which is why we use pop() instead of parent() over there, since it does not have a failure really, it just stays the same path if it does not have one.

Removed as well, since it should never happen.

Comments from Reviewable

@pronebird
Copy link
Contributor

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

talpid-core/src/tunnel/mod.rs, line 220 at r1 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

You can google "launching exe from a memory buffer"

This piece has been removed so I marked this thread as satisfied.

Comments from Reviewable

@faern
Copy link
Member

faern commented Mar 6, 2018

:lgtm:

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@jvff
Copy link
Contributor Author

jvff commented Mar 7, 2018

Should this change be described in the changelog? From my understanding this is mostly a developer visible feature, correct?

Review status: 1 of 2 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@faern
Copy link
Member

faern commented Mar 7, 2018

It might also be noticed by power users. And it is a change in functionality. But I agree it could be classified as slightly borderline. I like the part you added, it's good.

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@mvd-ows
Copy link
Contributor

mvd-ows commented Mar 7, 2018

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

@pronebird
Copy link
Contributor

Reviewed 1 of 1 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

Janito Vaqueiro Ferreira Filho added 3 commits March 8, 2018 07:16
Use executable path as plugin path
806ecbc
Remove unnecessary error types
48e6736 
Executable without parent directory should never happen. In either case,
the current working directory is used as a fallback and the error is
logged. This is the same procedure used for the resources directory.
Add OpenVPN plugin path fix to the changelog
f42ab66
@jvff jvff merged commit f42ab66 into master Mar 8, 2018
@jvff jvff deleted the fix-openvpn-plugin-path branch March 8, 2018 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@faern faern

@eriklarko eriklarko

@pronebird pronebird

@jvff jvff

@mvd-ows mvd-ows

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jvff @pronebird @faern @mvd-ows @eriklarko