dbus interface redesign #40

jk-ozlabs · 2024-05-28T09:27:26Z

The PR #39 is precipitating a few other dbus interface changes that would need to happen at the same time. Just documenting some of my plans here.

The introduction of the links object directly in the /xyz/openbmc_project/mctp level breaks the existing convention that this object only hosts the tree of <net>/<eid> endpoint objects. Since we're doing that, we should probably only do this with a proper API revision, which then triggers some other changes. Those changes then come full-circle and may influence #39 too, so some discussion & planning here is definitely warranted.

The progression of those changes is approximately:

the bus name and object paths probably shouldn't reference the openbmc project namespace, as they're not OpenBMC specific. (OpenBMC does define the MCTP.Endpoint interface though, so that is suitable). So we should shift those to au.com.codeconstruct to avoid that. This is something that has been on the backburner for a while, so a revised API is the right time to introduce this change.
then, since we're defining new interfaces, we should version those, and apply the dbus standards for object & interface naming
which means that the top-level au.com.codeconstruct.MCTP interface is also up for redesign. Currently, it hosts the SetupEndpoint (and similar) methods. However, it's possible that those methods aren't appropriate for the a mctpd configuration, in the case where we're not a bus owner for anything (conditional on: would we still be running mctpd on that node? My assumption that there is still value in running some control protocol interactions there)
So, it's probably appropriate to rename au.com.codeconstruct.MCTP to au.com.codeconstruct.MCTP.BusOwner1, and only have it present on objects that are appropriate representations of a bus owner...
... which would be the new links/<name> objects that we're introducing in mctpd: Add mctp/links/<linkName> D-Bus object #39 !

So, my proposal for a best-practices dbus layout would be:

`/au/com/codeconstruct/mctp1`

"entry point" object for the mctp tree. Currently hosts the top-level .MCTP interface, but that would be moved. So, no interfaces planned at present, but would be suitable for future top-level configuration

`/au/com/codeconstruct/mctp1/networks/<net>`

Top-level for each MCTP network. No interfaces at present.

`/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid>`

Endpoint object. Would host:

xyz.openbmc_project.MCTP.Endpoint: as exists currently, for our generic endpoint properties: EID, Supported Message Types etc
xyz.openbmc_project.Common.UUID: as exists currently, for UUIDs
au.com.codeconstruct.MCTP.Endpoint1: as exists currently, for mctpd control of endpoints (Remove, Recover, etc)

`/au/com/codeconstruct/mctp1/links/<name>`

Link object, as proposed by #39 (but see my bikeshed suggestion below). Would host:

au.com.codeconstruct.MCTP.Link1 (as proposed by Thu): generic link configuration, including Role property
au.com.codeconstruct.MCTP.BusOwner1: our new interface that replaces au.com.codeconstruct.MCTP, for control-protocol bus-owner functions (SetupEndpoint etc). Only present if Role == BusOwner.

Some of my open design points:

whether the BusOwner interface should be on the link object or somewhere else under the net
- if it is on the link, we would drop the link name from the method arguments. That we already have that link name as an argument suggests that the link is the correct abstraction level for the dbus interface functionality.
whether the link object location is suitable, or whether it should be under the networks/x object somehow.
- I have a preference for the current proposal, otherwise links will be difficult to find
- but each link is certainly specific one (and only one) net

And some options for bikeshed aficionados - there probably needs to be some tweaking of various names:

Plural or singular collections? /mctp1/networks/<net-id>/endpoints/<eid> or /mctp1/network/x/endpoint/y ?
- There is precedence for both, but plural seems a bit more common. I have a slight preference for singular, but common is probably better.
/mctp1/links/x or /mctp/interfaces/x ?
- "links" is a bit ambiguous, I prefer the latter

The text was updated successfully, but these errors were encountered:

ThuBaNguyen · 2024-05-28T11:36:21Z

Plural or singular collections? /mctp1/networks/<net-id>/endpoints/<eid> or /mctp1/network/x/endpoint/y ?

There is precedence for both, but plural seems a bit more common. I have a slight preference for singular, but common is probably better.

I prefer /mctp1/networks/<net-id>/endpoints/<eid>.

/mctp1/links/x or /mctp/interfaces/x ?

"links" is a bit ambiguous, I prefer the latter

I used /mctp1/links/x in #39 because I saw that we are using mctp link to setup one interface. I think /mctp/interfaces/x is good too.

About 2. then, since we're defining new interfaces, we should version those, and apply the dbus standards for object & interface naming, Do we really need the version? I prefer to use au.com.codeconstruct.MCTP.Endpoint than au.com.codeconstruct.MCTP.Endpoint1. au.com.codeconstruct.MCTP.Endpoint is clearer.

jk-ozlabs · 2024-05-28T13:23:29Z

Hi @ThuBaNguyen ,

Thanks for taking a look. Some responses:

I prefer /mctp1/networks/<net-id>/endpoints/<eid>.

OK, cool. Let's take that approach then.

"links" is a bit ambiguous, I prefer the latter

I used /mctp1/links/x in #39 because I saw that we are using mctp link to setup one interface. I think /mctp/interfaces/x is good too.

OK, I'll think about that one a bit more. The mctp link is for consistency with ip link.

Do we really need the version?

Yeah, it's necessary to prevent future API breakage (so we don't need a repeat of what we're doing now!).

It's fairly well-documented:

It is also a good idea to include the major version of the interface in the name, and increment it if incompatible changes are made

(https://dbus.freedesktop.org/doc/dbus-specification.html)

This is achieved by including a version number in each interface name, service name and object path which is incremented on every backwards-incompatible change.

(https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning)

and all of: http://0pointer.de/blog/projects/versioning-dbus.html

I prefer to use au.com.codeconstruct.MCTP.Endpoint than au.com.codeconstruct.MCTP.Endpoint1. au.com.codeconstruct.MCTP.Endpoint is clearer.

The names are certainly neater, but we do need the versioning for future compat.

jk-ozlabs · 2024-05-29T09:02:19Z

However, .Endpoint might be a special case here; it exists already, and we're not making any changes to the interface definition...

jk-ozlabs · 2024-05-29T09:03:33Z

although, we are standardising on the lower-case format for prefix component of the interfaces, so that probably warrants changing to stay uniform.

This change implements the dbus interface rework specified in #40. Essentially: this uses more standard bus, object and path names, and moves away from the xyz.openbmc_project namespace, and use au.com.codeconstruct (all lowercase) there instead, as we're not specificially an OpenBMC project. We also put collections of things (networks and endpoints) under a specifically-named object path, so we can introduce new collections alongside (interfaces) without compatibility issues This means: - the bus owner name is now au.com.codeconstruct.MCTP1 - interfaces are namespaced and versioned: - au.com.codeconstruct.MCTP.Endpoint1 - au.com.codeconstruct.MCTP.BusOwner1 - the top-level entrypoint path is versioned, as `/au/com/codeconstruct/mctp1` - the endpoint object tree is structured as `/au/com/codeconstruct/mctp1/networks/<n>/endpoints/<e>` Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs · 2024-05-30T08:48:04Z

Have just pushed a proposed (ie., draft) implementation up at main...dev/dbus-rework.

This uses the configuration branch as a base, as I intend to merge that beforehand.

jk-ozlabs · 2024-05-30T08:51:37Z

The plan from there would be:

merge mctpd: Add mctp/links/<linkName> D-Bus object #39
move the BusOwner1.DRAFT interface to the new interface objects introduced by that merge (and hence changing that dbus interface, as we would no longer need the MCTP interface name as an argument)
remove the .DRAFT suffix

ThuBaNguyen · 2024-05-31T01:36:14Z

The plan from there would be:

merge mctpd: Add mctp/links/<linkName> D-Bus object #39

move the BusOwner1.DRAFT interface to the new interface objects introduced by that merge (and hence changing that dbus interface, as we would no longer need the MCTP interface name as an argument)

remove the .DRAFT suffix

I'm planing to update the mctp:

Remove au.com.codeconstruct.MCTP from /xyz/openbmc_project/mctp, move it to /au/com/codeconstruct/mctp1/links/ and rename that interface to to au.com.codeconstruct.MCTP.BusOwner1
Change /xyz/openbmc_project/mctp to /au/com/codeconstruct/mctp1
/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid> will include xyz.openbmc_project.MCTP.Endpoint, xyz.openbmc_project.Common.UUID, au.com.codeconstruct.MCTP.Endpoint` as current implementation.
These change will be added to mctpd: Add mctp/links/<linkName> D-Bus object #39 as the first commit.

For #39, I will

Name the DBus object as /au/com/codeconstruct/mctp1/links/<name>
Rename au.com.codeconstruct.MCTP.Link to au.com.codeconstruct.MCTP.Link1
Add au.com.codeconstruct.MCTP.BusOwner1 to /au/com/codeconstruct/mctp1/links/<name>

Let me know your idea.

jk-ozlabs · 2024-05-31T02:09:40Z

Not sure I totally understand there, but:

I'm planing to update the mctp:

Remove au.com.codeconstruct.MCTP from /xyz/openbmc_project/mctp, move it to /au/com/codeconstruct/mctp1/links/ and rename that interface to to au.com.codeconstruct.MCTP.BusOwner1

The rename to BusOwner1 is already present in my branch. I'll handle moving that to the MCTP interface/x objects once your change adds those objects

Change /xyz/openbmc_project/mctp to /au/com/codeconstruct/mctp1

Also already done in my branch, but may need updating in your changes too

/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid> will include xyz.openbmc_project.MCTP.Endpoint, xyz.openbmc_project.Common.UUID, au.com.codeconstruct.MCTP.Endpoint` as current implementation.

This is done too.

These change will be added to mctpd: Add mctp/links/<linkName> D-Bus object #39 as the first commit.

Just rebase on top of my proposed branch for these.

For #39, I will

Name the DBus object as /au/com/codeconstruct/mctp1/links/<name>

Rename au.com.codeconstruct.MCTP.Link to au.com.codeconstruct.MCTP.Link1

Sounds good!

Add au.com.codeconstruct.MCTP.BusOwner1 to /au/com/codeconstruct/mctp1/links/<name>

I'll handle this as a change (ie, moving the dbus interface implementation) after #39 is merged.

ThuBaNguyen · 2024-05-31T02:17:51Z

I don't know that you already update the mctp code as your proposal in branch dev/dbus-rework.
I will update #39 base on this branch.

jk-ozlabs · 2024-05-31T02:21:15Z

Ah, and maybe interfaces rather than links. While both have some degree of namespace conflict, I think the "interfaces" term is more common for the actual network interface.

ThuBaNguyen · 2024-05-31T16:53:19Z

I don't know that you already update the mctp code as your proposal in branch dev/dbus-rework. I will update #39 base on this branch.

I'm in trouble with verifying branch dev/dbus-rework.
Below is my step to apply dev/dbus-rework

As commit mctpd: attempt a default configuration from /etc/mctpd.conf, I tried to create mctpd.conf and override the /etc/mctpd.conf with my configuration file.

# Mode: either bus-owner or endpoint
mode = "bus-owner"

# MCTP protocol configuration. Used for both endpoint and bus-owner modes.
[mctp]
message_timeout_ms = 250

After flash the OpenBMC image, I tried to cat /etc/mctpd.conf to confirm about the correctness of step 1.
I also tried to print ctx->mctp_timeout to confirm about the configuration is applied.

Check the journal log I saw below message

May 31 16:40:17 board systemd[1]: mctpd.service: start operation timed out. Terminating.
May 31 16:40:17 board systemd[1]: mctpd.service: Failed with result 'timeout'.
May 31 16:40:17 board systemd[1]: Failed to start MCTP control protocol daemon.

I don't change the mctpd.service

cat /lib/systemd/system/mctpd.service

[Unit]
Description=MCTP control protocol daemon
Wants=mctp-local.target
After=mctp-local.target

[Service]
Type=dbus
BusName=xyz.openbmc_project.MCTP
ExecStart=/usr/sbin/mctpd

[Install]
WantedBy=mctp.target

ThuBaNguyen · 2024-05-31T17:11:42Z

If I roll back to commit mctpd: attempt a default configuration from /etc/mctpd.conf(947205f)

There is no error regardless I don't add mctpd.conf.

jk-ozlabs · 2024-06-01T04:04:38Z

OK, sounds like I have a bug in the conf changes. I'll try with your config shortly.

ThuBaNguyen · 2024-06-04T00:29:11Z

OK, sounds like I have a bug in the conf changes. I'll try with your config shortly.

Any update @jk-ozlabs ?

jk-ozlabs · 2024-06-04T01:19:48Z

nope, it was the weekend! I'll check it out today.

jk-ozlabs · 2024-06-04T03:08:23Z

oh, looks like it isn't related to the config changes.

You have:

[Service]
Type=dbus
BusName=xyz.openbmc_project.MCTP
ExecStart=/usr/sbin/mctpd

You'll need to update BusName to match the new bus owner name.

I assume this is coming from the example in conf/mctpd.service, so I'll update that in the dbus-rework branch.

This change implements the dbus interface rework specified in #40. Essentially: this uses more standard bus, object and path names, and moves away from the xyz.openbmc_project namespace, and use au.com.codeconstruct (all lowercase) there instead, as we're not specificially an OpenBMC project. We also put collections of things (networks and endpoints) under a specifically-named object path, so we can introduce new collections alongside (interfaces) without compatibility issues This means: - the bus owner name is now au.com.codeconstruct.MCTP1 - interfaces are namespaced and versioned: - au.com.codeconstruct.MCTP.Endpoint1 - au.com.codeconstruct.MCTP.BusOwner1 - the top-level entrypoint path is versioned, as `/au/com/codeconstruct/mctp1` - the endpoint object tree is structured as `/au/com/codeconstruct/mctp1/networks/<n>/endpoints/<e>` Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs · 2024-06-04T03:13:38Z

so I'll update that in the dbus-rework branch.

ok, done.

This change implements the dbus interface rework specified in #40. Essentially: this uses more standard bus, object and path names, and moves away from the xyz.openbmc_project namespace, and use au.com.codeconstruct (all lowercase) there instead, as we're not specificially an OpenBMC project. We also put collections of things (networks and endpoints) under a specifically-named object path, so we can introduce new collections alongside (interfaces) without compatibility issues This means: - the bus owner name is now au.com.codeconstruct.MCTP1 - interfaces are namespaced and versioned: - au.com.codeconstruct.MCTP.Endpoint1 - au.com.codeconstruct.MCTP.BusOwner1 - the top-level entrypoint path is versioned, as `/au/com/codeconstruct/mctp1` - the endpoint object tree is structured as `/au/com/codeconstruct/mctp1/networks/<n>/endpoints/<e>` Signed-off-by: Jeremy Kerr <[email protected]>

ThuBaNguyen · 2024-06-04T04:40:02Z

New code fix the issue. Thanks.

This change implements the dbus interface rework specified in #40. Essentially: this uses more standard bus, object and path names, and moves away from the xyz.openbmc_project namespace, and use au.com.codeconstruct (all lowercase) there instead, as we're not specificially an OpenBMC project. We also put collections of things (networks and endpoints) under a specifically-named object path, so we can introduce new collections alongside (interfaces) without compatibility issues This means: - the bus owner name is now au.com.codeconstruct.MCTP1 - interfaces are namespaced and versioned: - au.com.codeconstruct.MCTP.Endpoint1 - au.com.codeconstruct.MCTP.BusOwner1 - the top-level entrypoint path is versioned, as `/au/com/codeconstruct/mctp1` - the endpoint object tree is structured as `/au/com/codeconstruct/mctp1/networks/<n>/endpoints/<e>` Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs mentioned this issue May 28, 2024

mctpd: Add mctp/links/<linkName> D-Bus object #39

Closed

jk-ozlabs self-assigned this May 28, 2024

jk-ozlabs mentioned this issue Jun 29, 2024

Add configuration file facility #42

Merged

jk-ozlabs mentioned this issue Jun 29, 2024

dbus rework #43

Merged

jk-ozlabs closed this as completed in #43 Jun 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dbus interface redesign #40

dbus interface redesign #40

jk-ozlabs commented May 28, 2024

ThuBaNguyen commented May 28, 2024

jk-ozlabs commented May 28, 2024

jk-ozlabs commented May 29, 2024

jk-ozlabs commented May 29, 2024

jk-ozlabs commented May 30, 2024

jk-ozlabs commented May 30, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented May 31, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented May 31, 2024

ThuBaNguyen commented May 31, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented Jun 1, 2024

ThuBaNguyen commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

ThuBaNguyen commented Jun 4, 2024 •

edited

Loading

dbus interface redesign #40

dbus interface redesign #40

Comments

jk-ozlabs commented May 28, 2024

/au/com/codeconstruct/mctp1

/au/com/codeconstruct/mctp1/networks/<net>

/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid>

/au/com/codeconstruct/mctp1/links/<name>

ThuBaNguyen commented May 28, 2024

jk-ozlabs commented May 28, 2024

jk-ozlabs commented May 29, 2024

jk-ozlabs commented May 29, 2024

jk-ozlabs commented May 30, 2024

jk-ozlabs commented May 30, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented May 31, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented May 31, 2024

ThuBaNguyen commented May 31, 2024

ThuBaNguyen commented May 31, 2024

jk-ozlabs commented Jun 1, 2024

ThuBaNguyen commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

jk-ozlabs commented Jun 4, 2024

ThuBaNguyen commented Jun 4, 2024 • edited Loading

`/au/com/codeconstruct/mctp1`

`/au/com/codeconstruct/mctp1/networks/<net>`

`/au/com/codeconstruct/mctp1/networks/<net>/endpoints/<eid>`

`/au/com/codeconstruct/mctp1/links/<name>`

ThuBaNguyen commented Jun 4, 2024 •

edited

Loading