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

[Stanislav Fomichev <sdf@google.com>]

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)