temp: Address feedback

internal/uvm/scsi/README.md

@@ -11,6 +11,9 @@ available to a VM, so that it is visible to the guest OS.
 We will generally use the term "mount" to refer to a SCSI device being mounted
 to a specific path, and with specific settings (e.g. encryption) inside
 a guest OS.
+Note that in `Manager`, "Mount" refers to the overall action of attaching and
+mounting in the guest OS. If we come up with a better name for this aggregate
+action, we should rename this.
 
 ## Principles
 
@@ -49,10 +52,10 @@ on the VM.
  used.
 - When it is asked to attach a new SCSI device, it will first check if the attachment
  already exists, and increase its refcount if so. If not, it will allocate a new
- controller/LUN slot for the attachment, and then call the `Attacher` to actually carry
+ controller/LUN slot for the attachment, and then call the `attacher` to actually carry
  out the attach operation.
-- When it is asked to detach a SCSI device, it first uses the `Unplugger` to carry out any
- guest-side remove actions, and then uses the `Attacher` to remove the attachment from
+- When it is asked to detach a SCSI device, it first uses the `unplugger` to carry out any
+ guest-side remove actions, and then uses the `attacher` to remove the attachment from
  the VM.
 - Tracks refcount on any attached SCSI devices, so that they are not detached until there
  has been a detach request for each matching attach request.
@@ -61,21 +64,27 @@ on the VM.
 - Tracks current SCSI devices mounted in the guest OS, and what mount options were applied.
 - When it is asked to mount a new SCSI device, it will first check if the mount (with same options)
  already exists, and increase its refcount if so. If not, it will track the new mount, and
- call the `Mounter` to actually carry out the guest mount operation.
+ call the `mounter` to actually carry out the guest mount operation.
 - Tracks refcount on any mounted SCSI devices, so that they are not unmounted until there has
  been an unmount request for each matching mount request.
 
-### Low level, the `Attacher`, `Mounter` and `Unplugger` types
+### Low level, the backend types
 
-These three are interfaces defined by the package. They provide a way for the client to
-provide low-level implementations of SCSI operations, passed in when constructing the
-`Manager` object.
+There are three interfaces that provide the low-level implementation to `attachManager` and
+`mountManager`. They are `attacher`, `mounter` and `unplugger`.
 
-These types are what carry out the actual operations on HCS or the bridge, for instance, to do
-a SCSI attachment or mount.
+- `attacher` provides the host-side operations of attach/detach of SCSI devices to a VM.
+- `mounter` provides the guest-side operations of mount/unmount of SCSI devices inside the
+ guest OS.
+- `unplugger` provides the guest-side operation of safely removing a SCSI device, before it
+ is detached from the VM.
 
-The `Unplugger` type exists to help with removing a SCSI device from the guest before detaching. It
-is used in the case of Linux containers to cleanly unplug the SCSI device.
+To improve clarity, these three interfaces are grouped into the external interfaces `HostBackend`,
+consisting of `attacher`, and `GuestBackend`, consisting of `mounter` and `unplugger`. There are
+also corresponding implementations of the external interfaces for HCS and direct bridge connections.
+
+The client passes in implementations of `HostBackend` and `GuestBackend` when instantiating the
+`Manager`.
 
 ## Future work
 
@@ -92,9 +101,11 @@ of future changes as we work in the package.
  (if any) verity info to use for a device should probably be determined by the client of the package.
 - Likewise, ACLing disks so a VM can access them should likely fall outside the purview of the package
  as well.
-- The various implementations of `Attacher`, `Mounter`, and `Unplugger` should probably live outisde
+- The implemnentations of `HostBackend` and `GuestBackend` should probably live outisde
  the package. There is no real reason for them to be defined in here, other than not having a clear
- place to put them instead right now.
+ place to put them instead right now. They would make more sense to live close to the concrete
+ implementation they depend upon. For instance, `bridgeGuestBackend` might make sense to be near the
+ GCS bridge connection implementation.
 - For unmounting, it is awkward to have to re-pass the mount configuration to the guest again. There is
  not a clear course of action if this differs from the original mount configuration, nor is this checked
  anywhere. It would be good for the guest to instead track what cleanup is needed for each mount point,

internal/uvm/scsi/attach.go

@@ -9,14 +9,14 @@ import (
 
 type attachManager struct {
  m sync.Mutex
- attacher Attacher
- unplugger Unplugger
+ attacher attacher
+ unplugger unplugger
  numControllers int
  numLUNsPerController int
  slots [][]*attachment
 }
 
-func newAttachManager(attacher Attacher, unplugger Unplugger, numControllers, numLUNsPerController int, reservedSlots []Slot) *attachManager {
+func newAttachManager(attacher attacher, unplugger unplugger, numControllers, numLUNsPerController int, reservedSlots []Slot) *attachManager {
  slots := make([][]*attachment, numControllers)
  for i := range slots {
  slots[i] = make([]*attachment, numLUNsPerController)
@@ -59,9 +59,13 @@ func (am *attachManager) attach(ctx context.Context, c *attachConfig) (controlle
  return 0, 0, err
  }
  if existed {
- <-att.waitCh
- if att.waitErr != nil {
- return 0, 0, att.waitErr
+ select {
+ case <-ctx.Done():
+ return 0, 0, ctx.Err()
+ case <-att.waitCh:
+ if att.waitErr != nil {
+ return 0, 0, att.waitErr
+ }
  }
  return att.controller, att.lun, nil
  }
@@ -124,6 +128,8 @@ func (am *attachManager) trackAttachment(c *attachConfig) (*attachment, bool, er
  if freeController == -1 {
  freeController = controller
  freeLUN = lun
+ // We don't break here, since we still might find an exact match for
+ // this attachment.
  }
  } else if reflect.DeepEqual(c, attachment.config) {
  attachment.refCount++

internal/uvm/scsi/backend.go

@@ -13,39 +13,51 @@ import (
  "github.com/Microsoft/hcsshim/internal/protocol/guestresource"
 )
 
-// The concrete types here (not the Attacher/Mounter/Unplugger interfaces) would be a good option
+// The concrete types here (not the HostBackend/GuestBackend interfaces) would be a good option
 // to move out to another package eventually. There is no real reason for them to live in
 // the scsi package, and it could cause cyclical dependencies in the future.
 
-// Attacher provides the low-level operations for attaching a SCSI device to a VM.
-type Attacher interface {
+// HostBackend provides the host-side operations needed to manage SCSI, such as attach/detach.
+type HostBackend interface {
+ attacher
+}
+
+// GuestBackend provides the guest-side operations needed to manage SCSI, such as mount/unmount
+// and unplug.
+type GuestBackend interface {
+ mounter
+ unplugger
+}
+
+// attacher provides the low-level operations for attaching a SCSI device to a VM.
+type attacher interface {
  attach(ctx context.Context, controller, lun uint, config *attachConfig) error
  detach(ctx context.Context, controller, lun uint) error
 }
 
-// Mounter provides the low-level operations for mounting a SCSI device inside the guest OS.
-type Mounter interface {
+// mounter provides the low-level operations for mounting a SCSI device inside the guest OS.
+type mounter interface {
  mount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error
  unmount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error
 }
 
-// Unplugger provides the low-level operations for cleanly removing a SCSI device inside the guest OS.
-type Unplugger interface {
+// unplugger provides the low-level operations for cleanly removing a SCSI device inside the guest OS.
+type unplugger interface {
  unplug(ctx context.Context, controller, lun uint) error
 }
 
-var _ Attacher = &hcsAttacher{}
+var _ attacher = &hcsHostBackend{}
 
-type hcsAttacher struct {
+type hcsHostBackend struct {
  system *hcs.System
 }
 
-// NewHCSAttacher provides an [Attacher] using a [hcs.System].
-func NewHCSAttacher(system *hcs.System) Attacher {
- return &hcsAttacher{system}
+// NewHCSHostBackend provides a [HostBackend] using a [hcs.System].
+func NewHCSHostBackend(system *hcs.System) HostBackend {
+ return &hcsHostBackend{system}
 }
 
-func (ha *hcsAttacher) attach(ctx context.Context, controller, lun uint, config *attachConfig) error {
+func (hhb *hcsHostBackend) attach(ctx context.Context, controller, lun uint, config *attachConfig) error {
  req := &hcsschema.ModifySettingRequest{
  RequestType: guestrequest.RequestTypeAdd,
  Settings: hcsschema.Attachment{
@@ -56,125 +68,99 @@ func (ha *hcsAttacher) attach(ctx context.Context, controller, lun uint, config
  },
  ResourcePath: fmt.Sprintf(resourcepaths.SCSIResourceFormat, guestrequest.ScsiControllerGuids[controller], lun),
  }
- return ha.system.Modify(ctx, req)
+ return hhb.system.Modify(ctx, req)
 }
 
-func (ha *hcsAttacher) detach(ctx context.Context, controller, lun uint) error {
+func (hhb *hcsHostBackend) detach(ctx context.Context, controller, lun uint) error {
  req := &hcsschema.ModifySettingRequest{
  RequestType: guestrequest.RequestTypeRemove,
  ResourcePath: fmt.Sprintf(resourcepaths.SCSIResourceFormat, guestrequest.ScsiControllerGuids[controller], lun),
  }
- return ha.system.Modify(ctx, req)
+ return hhb.system.Modify(ctx, req)
 }
 
-var _ Mounter = &bridgeMounter{}
+var _ mounter = &bridgeGuestBackend{}
+var _ unplugger = &bridgeGuestBackend{}
 
-type bridgeMounter struct {
+type bridgeGuestBackend struct {
  gc *gcs.GuestConnection
  osType string
 }
 
-// NewBridgeMounter provides a [Mounter] using a [gcs.GuestConnection].
+// NewBridgeGuestBackend provides a [GuestBackend] using a [gcs.GuestConnection].
 //
 // osType should be either "windows" or "linux".
-func NewBridgeMounter(gc *gcs.GuestConnection, osType string) Mounter {
- return &bridgeMounter{gc, osType}
+func NewBridgeGuestBackend(gc *gcs.GuestConnection, osType string) GuestBackend {
+ return &bridgeGuestBackend{gc, osType}
 }
 
-func (bm *bridgeMounter) mount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
- req, err := mountRequest(controller, lun, path, config, bm.osType)
+func (bgb *bridgeGuestBackend) mount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
+ req, err := mountRequest(controller, lun, path, config, bgb.osType)
  if err != nil {
  return err
  }
- return bm.gc.Modify(ctx, req)
+ return bgb.gc.Modify(ctx, req)
 }
 
-func (bm *bridgeMounter) unmount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
- req, err := unmountRequest(controller, lun, path, config, bm.osType)
+func (bgb *bridgeGuestBackend) unmount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
+ req, err := unmountRequest(controller, lun, path, config, bgb.osType)
  if err != nil {
  return err
  }
- return bm.gc.Modify(ctx, req)
-}
-
-var _ Mounter = &hcsMounter{}
-
-type hcsMounter struct {
- system *hcs.System
- osType string
+ return bgb.gc.Modify(ctx, req)
 }
 
-// NewHCSMounter provides a [Mounter] using a [hcs.System].
-//
-// osType should be either "windows" or "linux".
-func NewHCSMounter(system *hcs.System, osType string) Mounter {
- return &hcsMounter{system, osType}
-}
-
-func (hm *hcsMounter) mount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
- req, err := mountRequest(controller, lun, path, config, hm.osType)
+func (bgb *bridgeGuestBackend) unplug(ctx context.Context, controller, lun uint) error {
+ req, err := unplugRequest(controller, lun, bgb.osType)
  if err != nil {
  return err
  }
- return hm.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
-}
-
-func (hm *hcsMounter) unmount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
- req, err := unmountRequest(controller, lun, path, config, hm.osType)
- if err != nil {
- return err
+ if req.RequestType == "" {
+ return nil
  }
- return hm.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
+ return bgb.gc.Modify(ctx, req)
 }
 
-var _ Unplugger = &bridgeUnplugger{}
+var _ mounter = &hcsGuestBackend{}
+var _ unplugger = &hcsGuestBackend{}
 
-type bridgeUnplugger struct {
- gc *gcs.GuestConnection
+type hcsGuestBackend struct {
+ system *hcs.System
  osType string
 }
 
-// NewBridgeUnplugger provides an [Unplugger] using a [gcs.GuestConnection].
+// NewHCSGuestBackend provides a [GuestBackend] using a [hcs.System].
 //
 // osType should be either "windows" or "linux".
-func NewBridgeUnplugger(gc *gcs.GuestConnection, osType string) Unplugger {
- return &bridgeUnplugger{gc, osType}
+func NewHCSGuestBackend(system *hcs.System, osType string) GuestBackend {
+ return &hcsGuestBackend{system, osType}
 }
 
-func (bu *bridgeUnplugger) unplug(ctx context.Context, controller, lun uint) error {
- req, err := unplugRequest(controller, lun, bu.osType)
+func (hgb *hcsGuestBackend) mount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
+ req, err := mountRequest(controller, lun, path, config, hgb.osType)
  if err != nil {
  return err
  }
- if req.RequestType == "" {
- return nil
- }
- return bu.gc.Modify(ctx, req)
+ return hgb.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
 }
 
-var _ Unplugger = &hcsUnplugger{}
-
-type hcsUnplugger struct {
- system *hcs.System
- osType string
-}
-
-// NewHCSUnplugger provides an [Unplugger] using a [hcs.System].
-//
-// osType should be either "windows" or "linux".
-func NewHCSUnplugger(system *hcs.System, osType string) Unplugger {
- return &hcsUnplugger{system, osType}
+func (hgb *hcsGuestBackend) unmount(ctx context.Context, controller, lun uint, path string, config *mountConfig) error {
+ req, err := unmountRequest(controller, lun, path, config, hgb.osType)
+ if err != nil {
+ return err
+ }
+ return hgb.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
 }
 
-func (hu *hcsUnplugger) unplug(ctx context.Context, controller, lun uint) error {
- req, err := unplugRequest(controller, lun, hu.osType)
+func (hgb *hcsGuestBackend) unplug(ctx context.Context, controller, lun uint) error {
+ req, err := unplugRequest(controller, lun, hgb.osType)
  if err != nil {
  return err
  }
  if req.RequestType == "" {
  return nil
  }
- return hu.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
+ return hgb.system.Modify(ctx, &hcsschema.ModifySettingRequest{GuestRequest: req})
 }
 
 func mountRequest(controller, lun uint, path string, config *mountConfig, osType string) (guestrequest.ModificationRequest, error) {

internal/uvm/scsi/manager.go

@@ -39,26 +39,28 @@ type Slot struct {
  LUN uint
 }
 
-// NewManager creates a new Manager using the provided backend [Attacher],
-// [Mounter], and [Unplugger], as well as other configuration parameters.
+// NewManager creates a new Manager using the provided host and guest backends,
+// as well as other configuration parameters.
 //
 // guestMountFmt is the format string to use for mounts of SCSI devices in
 // the guest OS. It should have a single %d format parameter.
 //
 // reservedSlots indicates which SCSI slots to treat as already used. They
 // will not be handed out again by the Manager.
 func NewManager(
- attacher Attacher,
- mounter Mounter,
- unplugger Unplugger,
+ hb HostBackend,
+ gb GuestBackend,
  numControllers int,
  numLUNsPerController int,
  guestMountFmt string,
  reservedSlots []Slot,
-) *Manager {
- am := newAttachManager(attacher, unplugger, numControllers, numLUNsPerController, reservedSlots)
- mm := newMountManager(mounter, guestMountFmt)
- return &Manager{am, mm}
+) (*Manager, error) {
+ if hb == nil || gb == nil {
+ return nil, errors.New("host and guest backend must not be nil")
+ }
+ am := newAttachManager(hb, gb, numControllers, numLUNsPerController, reservedSlots)
+ mm := newMountManager(gb, guestMountFmt)
+ return &Manager{am, mm}, nil
 }
 
 // MountConfig specifies the options to apply for mounting a SCSI device in