Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] New provisioning commands (`fabric-new`, `endpoint-set`, `port-set`, `port-peer-new`, `port-peer-del`) do not reply with a full message; most only ACK on success.
- [Medium] The `fabric-new-params` nested attribute introduces an unnecessary extra wrapping nest for request parameters.
--
--- Patch [8]: [RFC PATCH 08/12] drm/fabric: add provisioning netlink uAPI ---
commit 99dd7c47ab1a93cd9f0234f8280e8cd261d71d5a
Author: Ilia Levi <[email protected]>
Subject: drm/fabric: add provisioning netlink uAPI
This commit extends the DRM fabric YNL specification with six privileged
provisioning commands for managing fabrics, endpoints, and ports.
> diff --git a/Documentation/netlink/specs/drm_fabric.yaml b/Documentation/netlink/specs/drm_fabric.yaml
> --- a/Documentation/netlink/specs/drm_fabric.yaml
> +++ b/Documentation/netlink/specs/drm_fabric.yaml
> @@ -108,6 +125,17 @@ attribute-sets:
> or DUMP reply carries its snapshot's generation; a notification
> carries the generation of its change. Statistics reads do not
> advance it.
> + # Provisioning request parameters
> + -
> + name: admin-state
> + type: u32
> + enum: admin-state
> + doc: Requested administrative state.
> + -
> + name: fabric-new-params
> + type: nest
> + nested-attributes: fabric-new-params
> + doc: Creation parameters for ``fabric-new`` (full nest).
[Severity: Medium]
Does this introduce an extra wrapping nest for the request parameters?
The generic netlink design rules for new families specify avoiding extra
wrapping nests. Wrapping the request parameters inside the fabric-new-params
nested attribute rather than placing them directly in the main drm-fabric
request attributes creates unnecessary API complexity.
[ ... ]
> @@ -421,6 +483,97 @@ operations:
> reply shape (full fabric nest).
> notify: fabric-get
>
> + -
> + name: endpoint-change-ntf
> + doc: |
> + Endpoint state change notification.
> + Reuses the endpoint-get reply shape (full endpoint nest).
> + notify: endpoint-get
> +
> + -
> + name: fabric-new
> + doc: Create a new empty fabric
> + attribute-set: drm-fabric
> + flags: [admin-perm]
> + do:
> + pre: drm-fabric-nl-pre-doit
> + post: drm-fabric-nl-post-doit
> + request:
> + attributes:
> + - fabric-new-params
> + reply:
> + attributes:
> + - fabric-id
[Severity: Medium]
Should this operation reply with a full message representing the object rather
than just the fabric-id?
The generic netlink design rules require that all operations, especially
NEW/ADD, reply with a full message carrying identifying information about the
new object. Replying only with the ID forces userspace to issue a separate
GET command to retrieve the full state.
[ ... ]
> + -
> + name: endpoint-set
> + doc: Attach/detach an endpoint to a fabric and/or set its admin state
> + attribute-set: drm-fabric
> + flags: [admin-perm]
> + do:
> + pre: drm-fabric-nl-endpoint-pre-doit
> + post: drm-fabric-nl-endpoint-post-doit
> + request:
> + attributes:
> + - endpoint-id
> + - dev-name
> + - bus-name
> + - fabric-id
> + - admin-state
[Severity: Medium]
Does this command intentionally omit a reply block?
Without a reply block, this state-mutating operation will only ACK on success.
Since once a command only ACKs, that behavior becomes unchangeable uAPI,
should this operation be updated to return a full message reflecting the
updated state?
[ ... ]
> + -
> + name: port-peer-new
> + doc: Set a port's neighbor
> + attribute-set: drm-fabric
> + flags: [admin-perm]
> + do:
> + pre: drm-fabric-nl-port-pre-doit
> + post: drm-fabric-nl-port-post-doit
> + request:
> + attributes:
> + - endpoint-id
> + - port-index
> + - peer
[Severity: Medium]
Like the endpoint-set command above, should this ADD operation define a
reply payload rather than defaulting to an ACK-only response?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.