On Tue, Jun 18, 2019 at 11:19:58PM -0400, Doug Ledford wrote:
> On Tue, 2019-06-18 at 22:45 -0400, Doug Ledford wrote:
> > On Tue, 2019-06-18 at 16:58 -0400, Doug Ledford wrote:
> > > On Tue, 2019-06-18 at 15:46 -0300, Jason Gunthorpe wrote:
> > > > On Tue, Jun 18, 2019 at 07:53:38PM +0300, Leon Romanovsky wrote:
> > > > > I have a very strong opinion about it.
> > > >
> > > > Then Doug should add the policies, here are the output values from
> > > > the
> > > > userspace:
> > > >
> > > > [RDMA_NLDEV_ATTR_CHARDEV] = { .type = NLA_U64 },
> > > > [RDMA_NLDEV_ATTR_CHARDEV_ABI] = { .type = NLA_U64 },
> > > > [RDMA_NLDEV_ATTR_DEV_INDEX] = { .type = NLA_U32 },
> > > > [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 },
> > > > [RDMA_NLDEV_ATTR_NODE_GUID] = { .type = NLA_U64 },
> > > > [RDMA_NLDEV_ATTR_UVERBS_DRIVER_ID] = { .type = NLA_U32 },
> > > > [RDMA_NLDEV_ATTR_CHARDEV_NAME] = { .type = NLA_NUL_STRING
> > > > },
> > > > [RDMA_NLDEV_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
> > > > [RDMA_NLDEV_ATTR_DEV_PROTOCOL] = { .type = NLA_NUL_STRING
> > > > },
> > > > [RDMA_NLDEV_ATTR_FW_VERSION] = { .type = NLA_NUL_STRING },
> > >
> > > Most of those were already in the policies. Only the four that you
> > > added to enum rdma_nldev_attr needed added to the policies, and two
> > > of
> > > them your patch already added. The only question I had is what the
> > > string length should be on ATTR_CHARDEV_NAME? I throw in the
> > > default
> > > of
> > > .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN, but I wasn't sure if that was
> > > right
> > > for this entry?
> >
> > First of all, let me say that this is a PITA. I'm thinking we need to
> > order all of the attributes in the policy array in alphabetical order
> > so
> > it's easier to tell what's there and what's missing.
> >
> > Also, I'm starting to not like adding items to the policy array before
> > they are actually acceptable as inputs. Sure, there's the argument
> > that
> > they won't get missed. But there's the counter argument that until
> > you
> > define them as an input in some netlink function that reads them, at
> > least for all of the string items, you can't actually set the real
> > string length limit. By adding them to the policy now, you make it a
> > guarantee that you'll end up changing the API under userspace later
> > when
> > they become a legitimate input. Not sure if that's actually
> > wise. The
> > u32 and u64, sure, that's fine. But the strings, that can cause
> > problems later.,
> >
>
> Maybe the best way to do string items if you add them ahead of time is
> to give them an input length of 1. Theoretically that's just the null
> byte. Then when you add a function to actually read the data, set the
> size to something real.
Like you said, it won't be needed till new input code is used, so from
API perspective, it doesn't matter what size will be. I liked your idea
to put it to be "1". It goes hand by hand to avoid possible addition
of input handlers without update of update nla_policy.
>
> Anyway, pushed to wip/dl-for-next, check it to make sure you're happy
> with the final results.
It looks good, thanks.
>
> --
> Doug Ledford <dledford@xxxxxxxxxx>
> GPG KeyID: B826A3330E572FDD
> Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
