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

many: abbreviated forms of disconnect #2066

Merged
merged 33 commits into from
Oct 4, 2016
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d02b967
interfaces: add {Plug,Slot}.Ref
zyga Apr 18, 2016
47a5852
interfaces: add ConnRef
zyga Apr 18, 2016
982d086
interfaces: return list of affected connections and snaps from Discon…
zyga Apr 18, 2016
e7ba90c
overlord/ifacestate: add support for abbreviated disconnect operations
zyga Apr 18, 2016
7ddcd7f
Merged branch 'fix-lp1571497' of https://github.com/zyga/snapd into d…
stolowski Sep 28, 2016
c1805ec
test fixes: consumer vs producer, snaps are ordering by name
stolowski Sep 28, 2016
faf4b63
Added disconnectPlugFromOsSlot.
stolowski Sep 30, 2016
ec4bcae
Unit tests for disconnect from core snap.
stolowski Sep 30, 2016
b44bb08
Added a spread test fro disconnect (WIP)
stolowski Sep 30, 2016
ea0ee46
Added disconnectPlugOrSlot
stolowski Sep 30, 2016
e9c2293
Cleanups, fixes
stolowski Sep 30, 2016
3e9348a
Fixes for special cases
stolowski Sep 30, 2016
a6f36a8
Fix help
stolowski Sep 30, 2016
6eefa28
Removed support for disconnect <snap>
stolowski Sep 30, 2016
612cc30
Merge branch 'master' of github.com:snapcore/snapd into disconnect-sh…
zyga Oct 1, 2016
f6101eb
WIP
zyga Oct 3, 2016
1ba25f2
interfaces: add ResolveDisconnectAll and DisconnectAll
zyga Oct 3, 2016
aae93f1
Merge branch 'master' of github.com:snapcore/snapd into disconnect-sh…
zyga Oct 3, 2016
853b227
interfaces: tweak error message in ResolveDisconnectAll
zyga Oct 3, 2016
fd17380
interfaces: fix typo
zyga Oct 3, 2016
7b9b6de
Merge branch 'master' of github.com:snapcore/snapd into disconnect-sh…
zyga Oct 4, 2016
fc5233a
interfaces: drop unlocked version of ResolveDisconnectAll
zyga Oct 4, 2016
5354595
interfaces: don't return anything from DisconnctAll
zyga Oct 4, 2016
d496ce0
interfaces: tweak documentation
zyga Oct 4, 2016
dc380e4
interfaces: fix typo
zyga Oct 4, 2016
1c0ff1f
interfaces: drop wildcard mode on repo.Connected
zyga Oct 4, 2016
1a1f189
interfaces: tweak error test
zyga Oct 4, 2016
9bb858f
interfaces: simplify and tweak disconnect methods and tests
zyga Oct 4, 2016
e34153d
overlord/interfaces: use new Disconnect/DisconnectAll
zyga Oct 4, 2016
3f88492
overlord/ifacestate: add more disconnect tests
zyga Oct 4, 2016
f653e81
daemon: adjust tests after message changes
zyga Oct 4, 2016
3a3bc02
interfaces: be extra paranoid about disconnect dispatch
zyga Oct 4, 2016
98e974c
overlord/ifacestate: return {Plug,Slot}Ref by value
zyga Oct 4, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions cmd/snap/cmd_disconnect.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,10 @@ $ snap disconnect <snap>:<plug> <snap>:<slot>

Disconnects the specific plug from the specific slot.

$ snap disconnect <snap>:<slot>
$ snap disconnect <snap>:<slot or plug>

Disconnects any previously connected plugs from the provided slot.

$ snap disconnect <snap>

Disconnects all plugs from the provided snap.
Disconnects everything from the provided plug or slot.
The snap name may be omitted for the OS snap.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OS snap/core snap/

`)

func init() {
Expand Down
9 changes: 3 additions & 6 deletions cmd/snap/cmd_disconnect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,10 @@ $ snap disconnect <snap>:<plug> <snap>:<slot>

Disconnects the specific plug from the specific slot.

$ snap disconnect <snap>:<slot>
$ snap disconnect <snap>:<slot or plug>

Disconnects any previously connected plugs from the provided slot.

$ snap disconnect <snap>

Disconnects all plugs from the provided snap.
Disconnects everything from the provided plug or slot.
The snap name may be omitted for the OS snap.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OS snap/core snap/


Application Options:
--version Print the version and exit
Expand Down
4 changes: 0 additions & 4 deletions cmd/snap/interfaces_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ func (sn *SnapAndName) UnmarshalFlag(value string) error {
case 2:
sn.Snap = parts[0]
sn.Name = parts[1]
// Reject ":name" (that is invalid)
if sn.Snap == "" {
sn.Name = ""
}
// Reject "snap:" (that should be spelled as "snap")
if sn.Name == "" {
sn.Snap = ""
Expand Down
1 change: 0 additions & 1 deletion cmd/snap/interfaces_common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ func (s *SnapAndNameSuite) TestUnmarshalFlag(c *C) {
c.Check(sn.Name, Equals, "")
// Invalid
for _, input := range []string{
":name", // Empty snap, makes no sense
"snap:", // Empty name, should be spelled as "snap"
":", // Both snap and name empty, makes no sense
"snap:name:more", // Name containing :, probably a typo
Expand Down
170 changes: 142 additions & 28 deletions interfaces/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,68 +327,182 @@ func (r *Repository) Connect(ref ConnRef) error {
// from all the slots found therein. It is not an error if there are no
// such plugs but it is still an error if the snap does not exist or has no
// slots at all.
func (r *Repository) Disconnect(plugSnapName, plugName, slotSnapName, slotName string) error {
//
// Disconnect returns the list of severed connections, the list of affected snaps
// and an error if one occurred.
func (r *Repository) Disconnect(plugSnapName, plugName, slotSnapName, slotName string) ([]ConnRef, []*snap.Info, error) {
r.m.Lock()
defer r.m.Unlock()

var conns []ConnRef
var snaps map[string]*snap.Info
var err error

switch {
case plugSnapName == "" && plugName == "" && slotName == "":
// Disconnect everything from slotSnapName
return r.disconnectEverythingFromSnap(slotSnapName)
case plugSnapName == "" && plugName == "":
// Disconnect everything from slotSnapName:slotName
return r.disconnectEverythingFromSlot(slotSnapName, slotName)
// Disconnect everything from either slot or plug
conns, snaps, err = r.disconnectPlugOrSlot(slotSnapName, slotName)
default:
return r.disconnectPlugFromSlot(plugSnapName, plugName, slotSnapName, slotName)
conns, snaps, err = r.disconnectPlugFromSlot(plugSnapName, plugName, slotSnapName, slotName)
}

if err != nil {
return nil, nil, err
}
// Flatten map of snaps into a sorted list
names := make([]string, 0, len(snaps))
for snapName := range snaps {
names = append(names, snapName)
}
sort.Strings(names)
result := make([]*snap.Info, 0, len(snaps))
for _, snapName := range names {
result = append(result, snaps[snapName])
}
return conns, result, nil
}

// disconnectEverythingFromSnap finds a specific snap and disconnects all the plugs connected to all the slots therein.
func (r *Repository) disconnectEverythingFromSnap(slotSnapName string) error {
if _, ok := r.slots[slotSnapName]; !ok {
return fmt.Errorf("cannot disconnect plug from snap %q, no such snap", slotSnapName)
// ResolveDisconnectAll finds all of the connections that would be disconnected by DisconnectAll()
Copy link
Contributor

@niemeyer niemeyer Oct 3, 2016

Choose a reason for hiding this comment

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

That sounds strange. It's this function that defines what DisconnectAll should disconnect, not the other way around. Suggestion for rename and documentation:

// Connected returns references for all connections that are currently
// established with the provided plug or slot.

// DisconnectAll disconnects all provided connection references.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated comment. Connected might be good enough, actually. This is much less magical than ResolveConnect, as there isn't much "resolving" going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

func (r *Repository) ResolveDisconnectAll(snapName, plugOrSlotName string) ([]ConnRef, error) {
r.m.Lock()
defer r.m.Unlock()

return r.unlockedResolveDisconnectAll(snapName, plugOrSlotName)
}

func (r *Repository) unlockedResolveDisconnectAll(snapName, plugOrSlotName string) ([]ConnRef, error) {
if snapName == "" {
// Look up the core snap if no snap name was given
switch {
case r.slots["core"] != nil:
snapName = "core"
case r.slots["ubuntu-core"] != nil:
snapName = "ubuntu-core"
default:
return nil, fmt.Errorf("cannot resolve disconnect, snap name is empty")
Copy link
Contributor

@niemeyer niemeyer Oct 4, 2016

Choose a reason for hiding this comment

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

s/cannot resolve disconnect, // (not a disconnection in this context)

}
}
for _, slot := range r.slots[slotSnapName] {
for plug := range r.slotPlugs[slot] {
var conns []ConnRef
if plugOrSlotName == "" {
// "wildcard" mode, affect all the plugs or slots in this snap
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any real use cases for this today? We don't want this in the CLI, so if there are no other use cases we should probably just error for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't seem that we do. I've simplified the code and removed this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed and simplified

for _, plug := range r.plugs[snapName] {
for _, slotRef := range plug.Connections {
connRef := ConnRef{PlugRef: plug.Ref(), SlotRef: slotRef}
conns = append(conns, connRef)
}
}
for _, slot := range r.slots[snapName] {
for _, plugRef := range slot.Connections {
connRef := ConnRef{PlugRef: plugRef, SlotRef: slot.Ref()}
conns = append(conns, connRef)
}
}
} else {
// Precise mode, affect specific plug or slot in this snap
if plug, ok := r.plugs[snapName][plugOrSlotName]; ok {
for _, slotRef := range plug.Connections {
connRef := ConnRef{PlugRef: plug.Ref(), SlotRef: slotRef}
conns = append(conns, connRef)
}
}
if slot, ok := r.slots[snapName][plugOrSlotName]; ok {
for _, plugRef := range slot.Connections {
connRef := ConnRef{PlugRef: plugRef, SlotRef: slot.Ref()}
conns = append(conns, connRef)
}
}
// Check if plugOrSlotName actually maps to anything
if len(conns) == 0 {
return nil, fmt.Errorf("snap %q has no plug or slot named %q", snapName, plugOrSlotName)
}
}
return conns, nil
}

// DisconnectAll disconnects all of the given connections, returning the list of affected snap names.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we returning a list of snap names? We asked the repository itself to resolve the list of affected connections for us, so we already have it at hand are actually handing it in. Feels strange to get this list out from this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, we don't have to get this back from the function. I was just transcribing things mechanically. I'll get rid of it and move the relevant logic to ifacestate where we actually need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also simplified

func (r *Repository) DisconnectAll(conns []ConnRef) []string {
r.m.Lock()
defer r.m.Unlock()

// Sever all the connections, keeping track of affected snaps
snaps := make(map[string]bool)
for _, conn := range conns {
plug := r.plugs[conn.PlugRef.Snap][conn.PlugRef.Name]
slot := r.slots[conn.SlotRef.Snap][conn.SlotRef.Name]
if plug != nil && slot != nil {
snaps[plug.Snap.Name()] = true
snaps[slot.Snap.Name()] = true
r.disconnect(plug, slot)
}
}
return nil
// Flatten map of affected snaps into a sorted list
names := make([]string, 0, len(snaps))
for snapName := range snaps {
names = append(names, snapName)
}
sort.Strings(names)
return names
}

// disconnectEverythingFromSlot finds a specific slot and disconnects all the plugs connected there.
func (r *Repository) disconnectEverythingFromSlot(slotSnapName, slotName string) error {
// Ensure that such slot exists
slot := r.slots[slotSnapName][slotName]
if slot == nil {
return fmt.Errorf("cannot disconnect plug from slot %q from snap %q, no such slot", slotName, slotSnapName)
func (r *Repository) disconnectPlugOrSlot(snapName, plugOrSlotName string) ([]ConnRef, map[string]*snap.Info, error) {
if snapName == "" {
// TODO: teach the repository about the OS snap
snapName = "ubuntu-core"
}

if r.slots[snapName][plugOrSlotName] == nil && r.plugs[snapName][plugOrSlotName] == nil {
err := fmt.Errorf("cannot disconnect plug or slot %q from snap %q, no such plug/slot", plugOrSlotName, snapName)
return nil, nil, err
}
for plug := range r.slotPlugs[slot] {

var conns []ConnRef
snaps := make(map[string]*snap.Info)
d := func(plug *Plug, slot *Slot) {
r.disconnect(plug, slot)
snaps[plug.Snap.Name()] = plug.Snap
snaps[slot.Snap.Name()] = slot.Snap
conns = append(conns, ConnRef{PlugRef: plug.Ref(), SlotRef: slot.Ref()})
}
return nil
if slot := r.slots[snapName][plugOrSlotName]; slot != nil {
for plug := range r.slotPlugs[slot] {
d(plug, slot)
}
}
if plug := r.plugs[snapName][plugOrSlotName]; plug != nil {
for slot := range r.plugSlots[plug] {
d(plug, slot)
}
}
return conns, snaps, nil
}

// disconnectPlugFromSlot finds a specific slot and plug and disconnects it.
func (r *Repository) disconnectPlugFromSlot(plugSnapName, plugName, slotSnapName, slotName string) error {
// disconnectPlugFromSlot finds a specific plug slot and plug and disconnects it.
func (r *Repository) disconnectPlugFromSlot(plugSnapName, plugName, slotSnapName, slotName string) ([]ConnRef, map[string]*snap.Info, error) {
// Ensure that such plug exists
plug := r.plugs[plugSnapName][plugName]
if plug == nil {
return fmt.Errorf("cannot disconnect plug %q from snap %q, no such plug", plugName, plugSnapName)
err := fmt.Errorf("cannot disconnect plug %q from snap %q, no such plug", plugName, plugSnapName)
return nil, nil, err
}
// Ensure that such slot exists
slot := r.slots[slotSnapName][slotName]
if slot == nil {
return fmt.Errorf("cannot disconnect plug from slot %q from snap %q, no such slot", slotName, slotSnapName)
err := fmt.Errorf("cannot disconnect plug from slot %q from snap %q, no such slot", slotName, slotSnapName)
return nil, nil, err
}
// Ensure that slot and plug are connected
if !r.slotPlugs[slot][plug] {
return fmt.Errorf("cannot disconnect plug %q from snap %q from slot %q from snap %q, it is not connected",
err := fmt.Errorf("cannot disconnect plug %q from snap %q from slot %q from snap %q, it is not connected",
plugName, plugSnapName, slotName, slotSnapName)
return nil, nil, err
}
r.disconnect(plug, slot)
return nil
conns := []ConnRef{{PlugRef: plug.Ref(), SlotRef: slot.Ref()}}
snaps := map[string]*snap.Info{
plug.Snap.Name(): plug.Snap,
slot.Snap.Name(): slot.Snap,
}
return conns, snaps, nil
}

// disconnect disconnects a plug from a slot.
Expand Down
Loading