On Sun, Jan 28, 2018 at 01:45:13PM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 28, 2018 at 11:17:24AM +0200, Leon Romanovsky wrote:
>
> > @@ -52,6 +54,11 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
> > [RDMA_NLDEV_ATTR_PORT_STATE] = { .type = NLA_U8 },
> > [RDMA_NLDEV_ATTR_PORT_PHYS_STATE] = { .type = NLA_U8 },
> > [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 },
> > + [RDMA_NLDEV_ATTR_RES_SUMMARY] = { .type = NLA_NESTED },
> > + [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY] = { .type = NLA_NESTED },
> > + [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME] = { .type = NLA_NUL_STRING,
> > + .len = 16 },
> > + [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR] = { .type = NLA_U64 },
>
> nla_policy is only used during kernel parsing, it shouldn't have
> anything the kernel will not accept as input, right? ie it should omit
> things that are output only?
The common practice for netlink code is to add nla_policy for every exposed
attribute. It achieves number of goals. First, future tools which will
send those attributes as inputs will be matched to ensure that UAPI
contract is still valid, despite the fact that it is not used in the
code. Second, once kernel developers will add needed implementation to
use those attributes as inputs, they are not supposed to touch this
policy, and it will be in sync with the output from the beginning - easy
review, easy maintenance.
>
> > static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
> > [RDMA_NLDEV_CMD_GET] = {
> > .doit = nldev_get_doit,
> > @@ -338,6 +485,10 @@ static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
> > .doit = nldev_port_get_doit,
> > .dump = nldev_port_get_dumpit,
> > },
> > + [RDMA_NLDEV_CMD_RES_GET] = {
> > + .doit = nldev_res_get_doit,
> > + .dump = nldev_res_get_dumpit,
> > + },
> > };
> >
> > void __init nldev_init(void)
> > diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> > index cc002e316d09..e0f5cdc81541 100644
> > +++ b/include/uapi/rdma/rdma_netlink.h
> > @@ -236,6 +236,11 @@ enum rdma_nldev_command {
> > RDMA_NLDEV_CMD_PORT_NEW,
> > RDMA_NLDEV_CMD_PORT_DEL,
> >
> > + RDMA_NLDEV_CMD_RES_GET, /* can dump */
> > + RDMA_NLDEV_CMD_RES_SET,
> > + RDMA_NLDEV_CMD_RES_NEW,
> > + RDMA_NLDEV_CMD_RES_DEL,
>
> Confused why all thse have get/set/new/del tuples and then don't
> implement all of them. Is there some reason for this?
>
> AFAIK netlink doesn't have any rules for numbering get/set/new/del, so
> why can't we just add when we need?
Maybe, it is cargo cult, but I followed devlink and devlink does such
to follow ip tool paradigm where every object can have those properties
(GET/SET/NEW/DEL).
The current declaration places all commands in one place.
Otherwise, this structure will be mixed with different commands.
>
> > @@ -303,6 +308,11 @@ enum rdma_nldev_attr {
> >
> > RDMA_NLDEV_ATTR_DEV_NODE_TYPE, /* u8 */
> >
> > + RDMA_NLDEV_ATTR_RES_SUMMARY, /* nested table */
> > + RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY, /* nested table */
> > + RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME, /* string */
> > + RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR, /* u64 */
>
> Had to look it up to figure out what CURR was..
>
> ENTRY_NUM_OBJECTS ?
I still didn't abandon my idea to provide ENTRY_MAX for the objects too.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
