Martijn van Duren <[email protected]> wrote: > On 8/14/19 4:48 PM, Claudio Jeker wrote: > > On Wed, Aug 14, 2019 at 04:37:48PM +0200, Martijn van Duren wrote: > >> On 8/14/19 3:35 PM, Claudio Jeker wrote: > >>> On Wed, Aug 14, 2019 at 02:37:14PM +0200, Martijn van Duren wrote: > >>>> This one stumped me for longer than I'd like to admit. > >>>> The problem I faced was that I expected > >>>> ber_scanf_elements(..., "{Se{", ...) to work if "e" is an nstring. > >>>> This was not the case because "e" doesn't increment ber and { is > >>>> tested against the nstring. > >>>> > >>>> Because I can see value in having it this way (retrieve the value and > >>>> element in the same run) I reckon we can keep it this way. > >>>> If you'd want to only have "e" you can "eS". > >>>> > >>>> Patch one documents this behaviour. > >>>> > >>>> If we rather have this behaviour changed, in that it always increments > >>>> that's patch two. This can be done safely: > >>>> $ grep -lRF '#include <ber.h>' *bin | while read file; do grep -H > >>>> ber_scanf_elements $file; done > >>>> usr.bin/snmp/snmpc.c: (void) ber_scanf_elements(pdu, "{Sdd{e", > >>>> &errorstatus, &errorindex, > >>>> usr.bin/snmp/snmpc.c: (void) ber_scanf_elements(pdu, "{Sdd{e", > >>>> &errorstatus, > >>>> usr.bin/snmp/snmpc.c: (void) ber_scanf_elements(pdu, "{Sdd{e", > >>>> &errorstatus, > >>>> usr.bin/snmp/snmpc.c: (void) > >>>> ber_scanf_elements(varbind, "{oe}", &noid, > >>>> usr.bin/snmp/snmpc.c: (void) ber_scanf_elements(pdu, "{Sdd{e", > >>>> &errorstatus, > >>>> usr.sbin/snmpd/traphandler.c: if (ber_scanf_elements(*req, "{dSe", > >>>> &vers, &elm) == -1) > >>>> usr.sbin/snmpd/traphandler.c: if (ber_scanf_elements(elm, > >>>> "{oSddd", > >>>> usr.sbin/snmpd/traphandler.c: if (ber_scanf_elements(elm, > >>>> "{SSS{e}}", &elm) == -1 || > >>>> usr.sbin/snmpd/traphandler.c: ber_scanf_elements(elm, > >>>> "{Sd}{So}", > >>>> usr.sbin/snmpd/traphandler.c: if (ber_scanf_elements(iter, > >>>> "{oe}", &oid, &elm) == -1) > >>>> usr.sbin/snmpctl/snmpclient.c: if (ber_scanf_elements(resp, > >>>> "{SS{SSSS{e}}", &s) != 0) > >>>> usr.sbin/snmpctl/snmpclient.c: if (ber_scanf_elements(s, > >>>> "{oe}", &sc->sc_oid, &e) != 0) > >>>> usr.sbin/snmpctl/snmpclient.c: if (ber_scanf_elements(resp, "{is{i", > >>>> &ver, &comn, &id) != 0) > >>>> Where "e" is always the last element checked. > >>>> Using "}" just raises the level, but doesn't check current ber to see if > >>>> it's the final element. > >>>> > >>>> OK for 1? > >>> > >>> Ok claudio@ > >>> > >>>> Good motivation for 2? > >>> > >>> In my opinion it would make sense to eat the element in 'e' like all other > >>> data retrivals do. It feels like this is easier to understand than having > >>> to use 'eS' for that. > >> > >> So which one do you prefer? If we document 1 it's somewhat final. > > > > Not necessarly final. Anyway, I would prefer 2. > > Still part of the diff should still go in since t and p still don't > > progress. > > > >> Also: Yes it's easier and more intuitive to do "e" instead of "eS", > >> but if you need both the ber_element and it's value it becomes > >> impossible to do so in a single run, resulting (in some cases) in > >> more code and more runtime. So I'm not convinced either one is > >> better. > > > > If you use 'e' it returns a ber_element this element includes the value > > (you just need to use one of the ber_get* functions on it). If you fetch > > the value then use 't' to get the class and type. Not sure if there is > > anything else that that would be needed. This is the reason why I think > > doing 2 is making the API better. > > > So that would be the following diff: > > Index: ber.c > =================================================================== > RCS file: /cvs/src/lib/libutil/ber.c,v > retrieving revision 1.12 > diff -u -p -r1.12 ber.c > --- ber.c 14 Aug 2019 04:48:13 -0000 1.12 > +++ ber.c 14 Aug 2019 14:49:57 -0000 > @@ -711,7 +711,7 @@ ber_scanf_elements(struct ber_element *b > e = va_arg(ap, struct ber_element **); > *e = ber; > ret++; > - continue; > + break; > case 'E': > i = va_arg(ap, long long *); > if (ber_get_enumerated(ber, i) == -1) > Index: ber_get_string.3 > =================================================================== > RCS file: /cvs/src/lib/libutil/ber_get_string.3,v > retrieving revision 1.5 > diff -u -p -r1.5 ber_get_string.3 > --- ber_get_string.3 21 May 2019 12:30:07 -0000 1.5 > +++ ber_get_string.3 14 Aug 2019 14:49:57 -0000 > @@ -107,6 +107,11 @@ and > .Sq t , > the type of the element is not checked. > For > +.Sq p , > +and > +.Sq t > +the pointer is not incremented to the next element. > +For > .Sq e , > a pointer to the element is stored in the corresponding pointer variable. > For >
This also seems the most sensible to me.
