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

Map mounting user's uid/gid by default (Fixes #331) #433

Merged
merged 2 commits into from
Oct 5, 2018
Merged

Conversation

Saviq
Copy link
Collaborator

@Saviq Saviq commented Oct 5, 2018

No description provided.

@Saviq Saviq requested a review from townsend2010 October 5, 2018 09:40
@Saviq Saviq added this to the 2018.10.2 milestone Oct 5, 2018
@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #433 into master will increase coverage by 0.16%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   66.28%   66.45%   +0.16%     
==========================================
  Files         141      141              
  Lines        5374     5389      +15     
==========================================
+ Hits         3562     3581      +19     
+ Misses       1812     1808       -4
Impacted Files Coverage Δ
include/multipass/sshfs_mount/sftp_server.h 0% <ø> (ø) ⬆️
src/sshfs_mount/sftp_server.cpp 87.61% <0%> (-0.79%) ⬇️
src/client/client.cpp 89.47% <100%> (+0.38%) ⬆️
src/client/cmd/mount.cpp 51% <100%> (+4.84%) ⬆️
include/multipass/logging/level.h 92.3% <0%> (+30.76%) ⬆️
src/logging/standard_logger.cpp 60% <0%> (+60%) ⬆️
include/multipass/logging/standard_logger.h 100% <0%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e43fed5...ac0b3cb. Read the comment docs.

fmt::format("{}:{} {}(): adding default uid/gid mapping", __FILE__, __LINE__, __FUNCTION__));
auto uid_entry = request.add_uid_maps();
uid_entry->set_host_uid(getuid());
uid_entry->set_instance_uid(-2);
Copy link
Contributor

Choose a reason for hiding this comment

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

This "-2" confused me until I read the sftp_server code. So -2 means no_id_info_available, ok. The idea of "set the instance id to: no id info available" is also peculiar. But I don't see a simple refactoring that will correct this.

For the short term can this magic "-2" be defined in a shared place as a constant, so we can type something like:

set_instance_uid(use_host_uid)


auto gid_entry = request.add_gid_maps();
gid_entry->set_host_gid(getgid());
gid_entry->set_instance_gid(-2);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

@gerboland gerboland left a comment

Choose a reason for hiding this comment

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

I think that's ok. Am curious why -2 was used in favour of -1, but I see no obvious reason why. @townsend2010 might know. But +1 from me

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Please run clang-format on this...I noticed some of the braces are not consistent with the rest of our code:)

Other than that, lgtm.

@Saviq Saviq force-pushed the map-mounting-user branch from baea33f to 4974b7a Compare October 5, 2018 12:38
@Saviq Saviq force-pushed the map-mounting-user branch from 4974b7a to ac0b3cb Compare October 5, 2018 12:40
@Saviq
Copy link
Collaborator Author

Saviq commented Oct 5, 2018

@townsend2010 done.

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Oct 5, 2018
433: Map mounting user's uid/gid by default (Fixes #331) r=townsend2010 a=Saviq



Co-authored-by: Michał Sawicz <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 5, 2018

Build succeeded

@bors bors bot merged commit ac0b3cb into master Oct 5, 2018
@bors bors bot deleted the map-mounting-user branch October 5, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants