ber.c: Document ber_scanf_element moving forward

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

ber.c: Document ber_scanf_element moving forward

Martijn van Duren-5
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?
Good motivation for 2?

martijn@

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 12:30:17 -0000
@@ -108,6 +108,12 @@ and
 the type of the element is not checked.
 For
 .Sq e ,
+.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
 .Sq S ,


2)
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 12:30:48 -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)

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Claudio Jeker
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.
 

> martijn@
>
> 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 12:30:17 -0000
> @@ -108,6 +108,12 @@ and
>  the type of the element is not checked.
>  For
>  .Sq e ,
> +.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
>  .Sq S ,
>
>
> 2)
> 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 12:30:48 -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)
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Martijn van Duren-5
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.
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.
>  
>> martijn@
>>

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Claudio Jeker
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.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Martijn van Duren-5
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

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Claudio Jeker
On Wed, Aug 14, 2019 at 04:51:10PM +0200, Martijn van Duren 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
>

OK claudio@

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: Document ber_scanf_element moving forward

Theo de Raadt-2
In reply to this post by Martijn van Duren-5
Martijn van Duren <[hidden email]> 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.