General purpose library for name/value pairs.

[Pawel Jakub Dawidek]

On Fri, Jul 05, 2013 at 08:10:40PM +0000, Poul-Henning Kamp wrote:
> In message <20130705195255.GB25842 at garage.freebsd.pl>, Pawel Jakub Dawidek writ
> es:
> 
> >To be honest I'd much prefer not to support duplicates, because arrays
> >of values are supported as well as nesting. Not supporting duplicates
> >would simplify implementation a bit.
> 
> So why do you support it ?

Before I proposed libnv here I did two things (which I find healthy):

1. I implemented this functionality in the past (in HAST and auditdistd)
   as internal functionality to gather some experience and learn about
   shortcomings. The previous implementation for example was focused on
   speed - this caused alignment issues and wasn't flexible. Later I
   realized that performance shouldn't be the top priority for this API.

2. I implemented consumers for this API. I use it in many places in my
   capsicum branch and I know it will fit HAST and auditdistd too.

And the point 2 actually showed that supporting duplicates has some
value. For example when I pass nvlist around for unrelated code to add
its arguments, I don't have to pass current counter, as everyone can add
its own argument under the same name, which is pretty convenient.

Not sure if this is good enough argument to support duplicates when you
take into account what it actually takes to support them:

- If receiver wants to be sure there are no duplicates, nvlist_recv()
  should probably take 'flags' argument, so the caller can declare at
  that point if he expects duplicates or not. This is currently not
  done.

- Without duplicates we could even try to get rid of nvpair_t
  altogether. nvpair_t is exposed mostly to support easy walking through
  nvlist because of duplicates. If this would be much rare operation,
  instead of doing:

	nvlist_t *nvl;
	nvpair_t *nvp;

	for (nvp = nvlist_first_nvpair(nvl); nvp != NULL;
	    nvp = nvlist_next_nvpair(nvl, nvp)) {
		/* ... */
	}

  one could do:

	nvlist_t *nvl;
	const char *name;
	int type;
	void *cookie;

	cookie = NULL;
	while ((name = nvlist_next(nvl, &type, &cookie)) != NULL) {
		/* ... */
	}

   Although passing nvpair around without even knowing its type might be
   also valuable sometimes.

I'm not really sure. I do want the API to be as simple as possible and
as easy to use as possible, but not too simple and too easy, so people
will keep hacking their own stuff. Opinions welcome.

> >> Also remember a function for debugging which renders a nvlist_t (into
> >> a sbuf ?)
> >
> >Currently I have:
> >
> >	void nvlist_dump(const nvlist_t *nvl, int fd);
> >	void nvlist_fdump(const nvlist_t *nvl, FILE *out);
> 
> I would just make one, and have it return an sbuf.  That way the
> same code can be used in kernel and userland, and people get to
> decide where it should end up and how.

You seem to ignored my comment about not willing to depend on sbuf,
because it will make adoption harder. Returning it as a regular string,
which you could add to sbuf is something I'd be more happy to add.

> >> Do consider having these functions in a variant with a default
> >> argument, so that people don't have to wrap each optioanl n/v pair
> >> in an if().
> >
> >Using default value to report a problem [...]
> 
> Uhm, it's not to report problems, it's to implement default values
> for non-specified names:
> 
> 	give me $foo or if there is no $foo, give me "32"
> 
> The return value should always be a "int where zero means OK" style
> error indicator, the returned value should go into a pointer arg.

What API do you imagine for that? And I wonder if it would be really
needed if I'd implement nvlist_spec I proposed. Then one would need to
do the checking only for optional nvpairs.

> >I was considering this as well, but there is a lot of functions as it is
> >now [...]
> 
> Yes, hate to say it, but that to me indicates that you're not quite on
> the right path.

:) Yes, I'm fully aware this doesn't look good:) Although I don't think
you can say the library is trying to do too much. The core functionality
is limited and well defined:

	nvlist_exists_<type>()
	nvlist_add_<type>()
	nvlist_move_<type>()
	nvlist_get_<type>()
	nvlist_take_<type>()
	nvlist_free_<type>()

	nvpair_create_<type>()
	nvpair_move_<type>()
	nvpair_get_<type>()

The number of functions (not the functionality) is pretty big, because
of multiple types and format-string- and va_list- versions.

> Maybe the basic n/v should just do strings, and interpretation of
> strings be a layer above ?

It would make getting values from the nvlist a hell - dealing with
strto* functions and checking if conversion succeeded, that just too
complex. If you look at the functionality it doesn't look that bad.
atomic(9) has the same "problem" with multiple types.

> >> [error string API]
> >Fine idea, but doesn't seem to directly related to libnv. 
> 
> No, but you'll have a hard time emitting meaningful errors from
> libnv without such a facility.
> 
> Right now everybody rolls their own, getaddrinfo has its own,
> GEOM has its own, netgraph has its own...
> 
> At some point we should call a stop, and I'd say "before libnv grows
> one too" is at good time :-)

Nice try, but libnv *itself* is my input to improve infrastrucutre, so
let's not get caried away, shall we?:)

Having nvlist_errmsg() still sounds useful.

> >There is only a problem with translating those messages, which we keep
> >avoiding.
> 
> You know ?  Screw that!   Having usable errors only in english is
> far better than having only "Invalid argument" in all the languages
> of the world.

Well, can't we do better than that? This argument goes both ways.
In the past I saw people blocking useful functionality going in because
it wasn't good enough for our standards and because it will stop someone
from doing it right. Many years from there are we are still without both
right and not-so-right-but-useful functionalities. In commercial
products having error messages visible through some webgui translated to
configured language if definiately desired.

Maybe having error message as an nvlist where format string (as one of
the nvpairs) can be translated and arguments are stored as nvpairs
instead of 'char *' would do the trick?:)

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://mobter.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20130706/56b7b32f/attachment.sig>