Re: [PATCH net-next v2 2/4] tools: ynl: user space helpers

[Stanislav Fomichev <sdf@google.com>]

On 06/05, Stanislav Fomichev wrote:
> On 06/04, Jakub Kicinski wrote:
> > Add "fixed" part of the user space Netlink Spec-based library.
> > This will get linked with the protocol implementations to form
> > a full API.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > v2: fix up kdoc
> > ---
> >  .../userspace-api/netlink/intro-specs.rst     |  79 ++
> >  tools/net/ynl/Makefile                        |  19 +
> >  tools/net/ynl/generated/Makefile              |  45 +
> >  tools/net/ynl/lib/Makefile                    |  28 +
> >  tools/net/ynl/lib/ynl.c                       | 901 ++++++++++++++++++
> >  tools/net/ynl/lib/ynl.h                       | 237 +++++
> >  tools/net/ynl/ynl-regen.sh                    |   2 +-
> >  7 files changed, 1310 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/net/ynl/Makefile
> >  create mode 100644 tools/net/ynl/generated/Makefile
> >  create mode 100644 tools/net/ynl/lib/Makefile
> >  create mode 100644 tools/net/ynl/lib/ynl.c
> >  create mode 100644 tools/net/ynl/lib/ynl.h
> > 
> > diff --git a/Documentation/userspace-api/netlink/intro-specs.rst b/Documentation/userspace-api/netlink/intro-specs.rst
> > index a3b847eafff7..bada89699455 100644
> > --- a/Documentation/userspace-api/netlink/intro-specs.rst
> > +++ b/Documentation/userspace-api/netlink/intro-specs.rst
> > @@ -78,3 +78,82 @@ to see other examples.
> >  The code generation itself is performed by ``tools/net/ynl/ynl-gen-c.py``
> >  but it takes a few arguments so calling it directly for each file
> >  quickly becomes tedious.
> > +
> > +YNL lib
> > +=======
> > +
> > +``tools/net/ynl/lib/`` contains an implementation of a C library
> > +(based on libmnl) which integrates with code generated by
> > +``tools/net/ynl/ynl-gen-c.py`` to create easy to use netlink wrappers.
> > +
> > +YNL basics
> > +----------
> > +
> > +The YNL library consists of two parts - the generic code (functions
> > +prefix by ``ynl_``) and per-family auto-generated code (prefixed
> > +with the name of the family).
> > +
> > +To create a YNL socket call ynl_sock_create() passing the family
> > +struct (family structs are exported by the auto-generated code).
> > +ynl_sock_destroy() closes the socket.
> > +
> > +YNL requests
> > +------------
> > +
> > +Steps for issuing YNL requests are best explained on an example.
> > +All the functions and types in this example come from the auto-generated
> > +code (for the netdev family in this case):
> > +
> > +.. code-block:: c
> > +
> > +   // 0. Request and response pointers
> > +   struct netdev_dev_get_req *req;
> > +   struct netdev_dev_get_rsp *d;
> > +
> > +   // 1. Allocate a request
> > +   req = netdev_dev_get_req_alloc();
> > +   // 2. Set request parameters (as needed)
> > +   netdev_dev_get_req_set_ifindex(req, ifindex);
> > +
> > +   // 3. Issues the request
> > +   d = netdev_dev_get(ys, req);
> > +   // 4. Free the request arguments
> > +   netdev_dev_get_req_free(req);
> > +   // 5. Error check (the return value from step 3)
> > +   if (!d) {
> > +	// 6. Print the YNL-generated error
> > +	fprintf(stderr, "YNL: %s\n", ys->err.msg);
> > +        return -1;
> > +   }
> > +
> > +   // ... do stuff with the response @d
> > +
> > +   // 7. Free response
> > +   netdev_dev_get_rsp_free(d);
> 
> General API question: do we have to have all those alloc/free calls?
> Why not have the following instead?
> 
> 	struct netdev_dev_get_req req;
> 	struct netdev_dev_get_rsp rsp;
> 	
> 	netdev_dev_get_req_set_ifindex(&req, ifindex);
> 	netdev_dev_get(ys, &req, &rsp);
> 
> You seem to be doing malloc(*rsp) anyway in netdev_dev_get, so
> why not push that choice (heap/stack) on the users?
> 
> (haven't looked too closely at the series, so maybe a stupid question)

Answering to myself: netdev_dev_get_rsp is a simple case. With more
involved responses, we might have variable data and pointers to
different sub-chunks. So having netdev_dev_get-like getters do the
allocations seems like a sensible option..