-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add utility function for sharing files into UVMs from the host #907
Conversation
c660de2
to
8c5eb94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@kevpar PTAL if you can, I'm curious what you're feedback on this change would be. |
internal/uvm/share.go
Outdated
if _, err := uvm.GetVSMBUvmPath(ctx, reqHostPath, readOnly); err == ErrNotAttached { | ||
// share file has not been added yet, add it now | ||
options := uvm.DefaultVSMBOptions(readOnly) | ||
vsmbShare, err := uvm.AddVSMB(ctx, reqHostPath, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably always call AddVSMB
, even if the share is already added. If we want to support removing a share in the future, we will want actual calls to AddVSMB
each time, so the ref-counting works out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Wrote this a while ago so I'm not sure why I added that extra check, but removed.
internal/uvm/share.go
Outdated
return err | ||
} | ||
defer func() { | ||
if retErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use err
for the return value or do we need the different name for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used retErr
so I didn't get confused but it's not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments, otherwise looks fine.
Signed-off-by: Kathryn Baldauf <[email protected]>
8c5eb94
to
020897b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm... again :)
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
Add utility function for sharing files into UVMs from the host
This PR refactors out sharing files in the UVM into a new call on the UtilityVM. This allows us to make a more generic call that can be used later on for adding files for both LCOW and WCOW.
This will be used later on in the dynamic resources work.
Signed-off-by: Kathryn Baldauf [email protected]