Date: Sat, 6 Jul 2013 21:41:25 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Poul-Henning Kamp <phk@phk.freebsd.dk> Cc: arch@FreeBSD.org Subject: Re: General purpose library for name/value pairs. Message-ID: <20130706194124.GE25842@garage.freebsd.pl> In-Reply-To: <60317.1373055040@critter.freebsd.dk> References: <20130704215329.GG1402@garage.freebsd.pl> <4818.1373008073@critter.freebsd.dk> <20130705195255.GB25842@garage.freebsd.pl> <60317.1373055040@critter.freebsd.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
--10jrOL3x2xqLmOsH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 05, 2013 at 08:10:40PM +0000, Poul-Henning Kamp wrote: > In message <20130705195255.GB25842@garage.freebsd.pl>, Pawel Jakub Dawide= k writ > es: >=20 > >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. >=20 > 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 =3D nvlist_first_nvpair(nvl); nvp !=3D NULL; nvp =3D nvlist_next_nvpair(nvl, nvp)) { /* ... */ } one could do: nvlist_t *nvl; const char *name; int type; void *cookie; cookie =3D NULL; while ((name =3D nvlist_next(nvl, &type, &cookie)) !=3D 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); >=20 > 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 [...] >=20 > Uhm, it's not to report problems, it's to implement default values > for non-specified names: >=20 > give me $foo or if there is no $foo, give me "32" >=20 > 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 [...] >=20 > 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.=20 >=20 > No, but you'll have a hard time emitting meaningful errors from > libnv without such a facility. >=20 > Right now everybody rolls their own, getaddrinfo has its own, > GEOM has its own, netgraph has its own... >=20 > 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. >=20 > 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 =66rom 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?:) --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://mobter.com --10jrOL3x2xqLmOsH Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iEYEARECAAYFAlHYcuQACgkQForvXbEpPzRf3wCeKHmVbnDmcYCvEqrgL+tEky82 i+YAn2l2d5ytptJMbV3qXOBB1Jhhqswr =bt/a -----END PGP SIGNATURE----- --10jrOL3x2xqLmOsH--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130706194124.GE25842>