Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/builtin: improve the bluez interface #1078
Conversation
and others
added some commits
Apr 17, 2016
jdstrand
commented on interfaces/builtin/bluez.go in 16228ad
Apr 26, 2016
•
Edited 1 time
-
jdstrand
Apr 26, 2016
-
Apr 26, 2016
|
This is not sufficient because it turns into @SLOT_SECURITY_TAG_GLOB@ 'snap.bluez.*'. Granted, it is better than hardcoding but we need this to actually be: 'snap.bluez.' where '' is whatever service in the slot snap's 'apps' is being connected to. Fine to commit as is, but please add a FIXME stating this needs to be snap... |
|
This would have to be a list of apps because (it's possible) that bluez snap will have many apps bound to this slot. Is that something we're okay with? |
jdstrand
replied
Apr 26, 2016
•
Edited 1 time
-
jdstrand
Apr 26, 2016
-
Apr 26, 2016
|
As for list, yes. Here would be the various patterns as I see them:
|
|
Oh, nice, I like {app1,...} approach. I'll update this pull request to use this. |
jdstrand
reviewed
Apr 26, 2016
| } | ||
| - fmt.Fprintf(buf, appName) | ||
| + fmt.Fprintf(buf, "}") | ||
| + new = buf.Bytes() | ||
| } |
jdstrand
Apr 26, 2016
•
Edited 1 time
-
jdstrand
Apr 26, 2016
Contributor
-
Apr 26, 2016
All this logic looks fine and I fully agree with the three cases. Personal preference would be to not use a case statement here since to my eyes it isn't as clear (if/else if/else would be clearer to me), but I won't block on that.
|
@zyga 's latest commit LGTM wrt AppArmor policy syntax and semantics. |
mvo5
reviewed
Apr 27, 2016
| + var new []byte | ||
| + switch { | ||
| + case len(slot.Apps) == 1: | ||
| + for appName := range slot.Apps { |
mvo5
Apr 27, 2016
Collaborator
I see len(slot.Apps) == 1 and then a range slot.Apps. That loop seems to be not required if we know the size is just one? Or am I missing something?
mvo5
reviewed
Apr 27, 2016
| + appNames = append(appNames, appName) | ||
| + } | ||
| + sort.Strings(appNames) | ||
| + for i, appName := range appNames { |
mvo5
Apr 27, 2016
Collaborator
I wonder if this is more readable with: fmt.Fprintf(buf, "snap.%s.{%s}",slot.Snap.Name(), strings.Join(appNames, ",")). Or is the loop doing something that I miss that strings.Join() is not doing?
morphis
Apr 27, 2016
Contributor
We should also move this code somewhere else as I will otherwise copy&paste it to the networkmanager interface where we have to do the same thing.
zyga
Apr 27, 2016
Contributor
Yes, I was thinking the same thing. I'll move that to dbus support code in the same package.
zyga
Apr 27, 2016
Contributor
@mvo5 I don't have any preference, I can use join if you think that would look better.
zyga
added some commits
Apr 28, 2016
|
This is ready for final review, I think we can merge it |
niemeyer
reviewed
Apr 29, 2016
| case interfaces.SecurityAppArmor: | ||
| - return bluezConnectedPlugAppArmor, nil | ||
| + old := []byte("@SLOT_SECURITY_TAGS@") |
niemeyer
Apr 29, 2016
Contributor
We already have a pattern for replacements. Can we please stick to a single one of those so we're not introducing these for each independent feature? (@foo@ vs. ###FOO### vs. {{foo}} vs ...).
zyga
Apr 29, 2016
Contributor
Ah, sorry, you are completely right. I guess I was looking at apparmor for too long. I'll fix this quickly.
niemeyer
reviewed
Apr 29, 2016
| +// - "snap.$snap.$app" if there is exactly one app bound | ||
| +// - "snap.$snap.{$app1,...$appN}" if there are some, but not all, apps bound | ||
| +// - "snap.$snap.*" if all apps are bound to the slot | ||
| +func slotAppLabelExpr(slot *interfaces.Slot) []byte { |
niemeyer
reviewed
Apr 29, 2016
| + snapName := slot.Snap.Name() | ||
| + if len(slot.Apps) == 1 { | ||
| + for appName := range slot.Apps { | ||
| + return []byte(fmt.Sprintf("snap.%s.%s", snapName, appName)) |
niemeyer
Apr 29, 2016
Contributor
Let's please use a single bytes.Buffer, and have a single result at the end with return buf.Bytes().
zyga
added some commits
Apr 29, 2016
niemeyer
reviewed
Apr 29, 2016
| + } | ||
| + appNames := make([]string, 0, len(slot.Apps)) | ||
| + for appName := range slot.Apps { | ||
| + appNames = append(appNames, appName) |
niemeyer
Apr 29, 2016
Contributor
buf.WriteByte('{')
for appName := range slot.Apps {
buf.WriteString(appName)
buf.WriteByte(',')
}
buf.Truncate(buf.Len()-1)
buf.WriteByte('}')
zyga
Apr 29, 2016
•
Edited 1 time
-
zyga
Apr 29, 2016
Contributor
-
Apr 29, 2016
Ha, if you look at the history, this is very similar to what I did earlier :) (see 6e730c1)
zyga
Apr 29, 2016
Contributor
@niemeyer tweaked but kept the sorted output, I think it's nice to be deterministic in this case.
zyga
added some commits
Apr 29, 2016
|
LGTM |
zyga commentedApr 26, 2016
No description provided.