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

Get data directory value at app startup (Android) #6342

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Jun 12, 2024

Remove APP_PATH from mullvad-paths on Android since it should not be a constant value. Instead, it is passed down from the Android app startup. As it turns out, it is really ever used for pointing to the RPC socket in use.


This change is Reviewable

Copy link

linear bot commented Jun 12, 2024

@MarkusPettersson98 MarkusPettersson98 requested review from dlon and Rawa June 12, 2024 12:08
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

@albin-mullvad mentioned that we also should update the README.md since it describes the "hard-coded" path.

See:
https://github.com/mullvad/mullvadvpn-app?tab=readme-ov-file#file-paths-used-by-mullvad-vpn-app

Data Directory (usually, /data/data/net.mullvad.mullvadvpn/)
https://developer.android.com/reference/android/content/Context#getDataDir()

Files Directory (usually, /data/data/net.mullvad.mullvadvpn/files)
https://developer.android.com/reference/android/content/Context#getFilesDir()

Cache Directory (usually, /data/data/net.mullvad.mullvadvpn/cache)
https://developer.android.com/reference/android/content/Context#getCacheDir()

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


mullvad-jni/src/lib.rs line 244 at r1 (raw file):

        data_dir,
        cache_dir,
        files_dir,

Just a small remark, should these be in the same order as above? data, files, cache.
or does it conflict with how it works on other platforms?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

The paths might be different depending on the device, but this is in broad terms for the normal use case correct. However, if the user make use of work profiles or similar features the paths can be different.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


mullvad-jni/src/lib.rs line 244 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Just a small remark, should these be in the same order as above? data, files, cache.
or does it conflict with how it works on other platforms?

They might as well 😊

@dlon dlon requested a review from Rawa June 12, 2024 14:43
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 16 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @Rawa)


mullvad-daemon/src/management_interface.rs line 1042 at r2 (raw file):

                server_abort_rx.into_future().await;
            },
            &rpc_socket_path,

Nit: I think & can be removed.


mullvad-paths/src/lib.rs line 75 at r2 (raw file):

}

#[cfg(not(target_os = "android"))]

Do we not only ever use mullvad_paths for get_rpc_socket_path? Could we not conditionally compile all of this code and just define the socket path in the one module where it's called on Android?

@MarkusPettersson98 MarkusPettersson98 force-pushed the provide-data-dir-path-to-daemon-during-initialization-des-993 branch from 264e6a4 to 12620fb Compare June 13, 2024 10:55
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 17 of 22 files reviewed, 1 unresolved discussion (waiting on @dlon and @Rawa)


mullvad-daemon/src/management_interface.rs line 1042 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: I think & can be removed.

Done


mullvad-paths/src/lib.rs line 75 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Do we not only ever use mullvad_paths for get_rpc_socket_path? Could we not conditionally compile all of this code and just define the socket path in the one module where it's called on Android?

Solved this by not compiling mullvad-paths for Android at all. Instead, the path to the RPC socket is passed from the Android client, which means that the daemon does not have to care about it at all 😊

@Rawa Rawa requested a review from dlon June 13, 2024 15:11
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 9 files at r3, 5 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


mullvad-paths/src/lib.rs line 75 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Solved this by not compiling mullvad-paths for Android at all. Instead, the path to the RPC socket is passed from the Android client, which means that the daemon does not have to care about it at all 😊

Nice!

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 9 files at r3, 5 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


README.md line 357 at r5 (raw file):

| macOS | `/Library/Caches/mullvad-vpn/` |
| Windows | `C:\ProgramData\Mullvad VPN\cache` |
| Android | [`getFilesDir()`](https://developer.android.com/reference/android/content/Context#getFilesDir())  |

Is filesDir correct? I think we use the cache dir in this case?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


README.md line 357 at r5 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is filesDir correct? I think we use the cache dir in this case?

Nope, getCacheDir() should be the one

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Besides the getCacheDir remark it lgtm

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


README.md line 357 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Nope, getCacheDir() should be the one

Since @MarkusPettersson98 is on vacay, I pushed a fix for this.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Remove `APP_PATH` from `mullvad-paths` on Android since it should
not be a constant value. Instead, it is passed down from the Android app
startup. As it turns out, it is really ever used for pointing to the RPC
socket in use.
@MarkusPettersson98 MarkusPettersson98 force-pushed the provide-data-dir-path-to-daemon-during-initialization-des-993 branch from 13c3c40 to 5c16eff Compare June 17, 2024 07:50
@MarkusPettersson98 MarkusPettersson98 merged commit 778e529 into main Jun 17, 2024
48 of 49 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the provide-data-dir-path-to-daemon-during-initialization-des-993 branch June 17, 2024 08:00
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