Hi Johan, On Thu, 2018-11-08 at 12:45 +0200, Johan Hedberg wrote: > Hi Inga, > > At first glance this looks quite good to me, just some cosmetic > comments: > > > On 7 Nov 2018, at 2.49, Inga Stotland <inga.stotland@xxxxxxxxx> > > wrote: > > +Service org.bluez.mesh1 > > I don’t think we’ve had the habit of versioning well-known bus names > so far. Or did you see this somewhere? So I’d just leave away the > version here. > From D-Bus Api Design Guidelines: ( https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning): "In summary, version numbers should be included in all service names, interface names and object paths: com.example.MyService1 com.example.MyService1.InterestingInterface com.example.MyService1.OtherInterface /com/example/MyService1/Manager /com/example/MyService1/OtherObject " I chose to do the name versioning based on the official guidelines. However, if we want to preserve the historical bluez approach, I don't mind changing it. > > +Interface org.bluez.mesh1.Network > > The way the existing interfaces do versioning is to have the version > at the very end, so this should be org.bluez.mesh.Network1 > > > +Object path /org/bluez/mesh1 > > I don’t think we have the habit of versioning object paths either. > They’re anyway (for the most part) irrelevant - it’s the interfaces > an object has that defines its type. > > > > + PossibleErrors: > > + org.bluez.mesh1.Error.Failed, > > + org.bluez.mesh1.Error.Canceled, > > + org.bluez.mesh1.Error.InvalidArguments > > + org.bluez.mesh1.Error.Timeout > > We’ve never versioned errors either, or did you see this done > somewhere? So just leave it out from here. So you can see a number of services following the above mentioned D-Bus API Guidelines by running d-feet: e..g., org.freedesktop.GeoClue2 or org.freedesktop.UDisks2 or org.fedoraprojectFirewallD1 > > > + (object node, array{byte, array{(uint16, dict}} > > configuration) Attach( > > + > > object app_defined_root, uint64 token) > > > > This is a bit hard to read due to the long & complex return value. > I’d suggest doing the line break before “Attatch” (a common C coding- > style as well). Agree > > > + void Send(object element_path, uint16_t destination, > > uint16 key_index, > > + array{b > > yte} data) > > Minor inconsistency: should be uint16 instead of uint16_t Agree > > Johan Thank you for the review. Regards, Inga
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
