ber.c: simplify ber_read()

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

ber.c: simplify ber_read()

Jeremie Courreges-Anglas-2

When looking at the recent changes in this file I noticed that the
presence of both ber_read() and ber_readbuf() was due to an incomplete
cleanup from my part.

  revision 1.32
  date: 2018/02/08 18:02:06;  author: jca;  state: Exp;  lines: +6 -22;  commitid: YNQMco5lCS8YEifQ;
  Kill ber.c support for direct fd read/writes

  This mechanism is already unused and annotated with lots of XXX's, no
  need to keep it around.  ok claudio@

I think the recent changes would have been easier if the code had been
further simplified.  Here's a shot at it:
- stop looping in ber_read(), there is no fd read to handle any more so
  calling ber_readbuf() once is enough
- amend the condition which decides whether to return -1/ECANCELED:
  - it makes little sense to pass a buffer length size of zero to
  ber_read(), but then we should probably return 0 and not -1/ECANCELED
  - I don't think we should perform partial reads: either the caller
  function has the data it asked for, or nothing.  Allowing partial
  reads means that we're putting the burden of handling an incomplete
  buffer on the caller.
- inline what remains of ber_readbuf() in ber_read()

regress tests for ldapd and snmpd pass, it would be cool to have test
reports from people who actually use those programs (ldap, ldapd, snmpd
and ypldap).

Looking for reviews and oks.  The diff is not that long but I can split
it in smaller parts if it helps.


Index: usr.bin/ldap/ber.c
===================================================================
RCS file: /d/cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.11
diff -u -p -r1.11 ber.c
--- usr.bin/ldap/ber.c 4 Jul 2018 15:21:24 -0000 1.11
+++ usr.bin/ldap/ber.c 6 Jul 2018 11:50:24 -0000
@@ -31,8 +31,6 @@
 
 #include "ber.h"
 
-#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
-
 #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
 #define BER_TYPE_SINGLE_MAX 30
 #define BER_TAG_MASK 0x1f
@@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
     int *cstruct);
 static ssize_t get_len(struct ber *b, ssize_t *len);
 static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
-static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
 static ssize_t ber_getc(struct ber *b, u_char *c);
 static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
 
@@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
  return totlen;
 }
 
-static ssize_t
-ber_readbuf(struct ber *b, void *buf, size_t nbytes)
-{
- size_t sz, len;
-
- if (b->br_rbuf == NULL)
- return -1;
-
- sz = b->br_rend - b->br_rptr;
- len = MINIMUM(nbytes, sz);
- if (len == 0) {
- errno = ECANCELED;
- return (-1); /* end of buffer and parser wants more data */
- }
-
- bcopy(b->br_rptr, buf, len);
- b->br_rptr += len;
- b->br_offs += len;
-
- return (len);
-}
-
 void
 ber_set_readbuf(struct ber *b, void *buf, size_t len)
 {
@@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
- return ber_readbuf(b, c, 1);
+ return ber_read(b, c, 1);
 }
 
 static ssize_t
 ber_read(struct ber *ber, void *buf, size_t len)
 {
- u_char *b = buf;
- ssize_t r, remain = len;
+ size_t sz;
 
- while (remain > 0) {
- r = ber_readbuf(ber, b, remain);
- if (r == -1)
- return -1;
- b += r;
- remain -= r;
+ if (ber->br_rbuf == NULL)
+ return -1;
+
+ sz = ber->br_rend - ber->br_rptr;
+ if (len > sz) {
+ errno = ECANCELED;
+ return -1; /* parser wants more data than available */
  }
- return (b - (u_char *)buf);
+
+ bcopy(ber->br_rptr, buf, len);
+ ber->br_rptr += len;
+ ber->br_offs += len;
+
+ return len;
 }
 
 int
Index: usr.sbin/ldapd/ber.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.21
diff -u -p -r1.21 ber.c
--- usr.sbin/ldapd/ber.c 4 Jul 2018 15:21:24 -0000 1.21
+++ usr.sbin/ldapd/ber.c 6 Jul 2018 11:47:55 -0000
@@ -31,8 +31,6 @@
 
 #include "ber.h"
 
-#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
-
 #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
 #define BER_TYPE_SINGLE_MAX 30
 #define BER_TAG_MASK 0x1f
@@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
     int *cstruct);
 static ssize_t get_len(struct ber *b, ssize_t *len);
 static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
-static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
 static ssize_t ber_getc(struct ber *b, u_char *c);
 static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
 
@@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
  return totlen;
 }
 
-static ssize_t
-ber_readbuf(struct ber *b, void *buf, size_t nbytes)
-{
- size_t sz, len;
-
- if (b->br_rbuf == NULL)
- return -1;
-
- sz = b->br_rend - b->br_rptr;
- len = MINIMUM(nbytes, sz);
- if (len == 0) {
- errno = ECANCELED;
- return (-1); /* end of buffer and parser wants more data */
- }
-
- bcopy(b->br_rptr, buf, len);
- b->br_rptr += len;
- b->br_offs += len;
-
- return (len);
-}
-
 void
 ber_set_readbuf(struct ber *b, void *buf, size_t len)
 {
@@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
- return ber_readbuf(b, c, 1);
+ return ber_read(b, c, 1);
 }
 
 static ssize_t
 ber_read(struct ber *ber, void *buf, size_t len)
 {
- u_char *b = buf;
- ssize_t r, remain = len;
+ size_t sz;
 
- while (remain > 0) {
- r = ber_readbuf(ber, b, remain);
- if (r == -1)
- return -1;
- b += r;
- remain -= r;
+ if (ber->br_rbuf == NULL)
+ return -1;
+
+ sz = ber->br_rend - ber->br_rptr;
+ if (len > sz) {
+ errno = ECANCELED;
+ return -1; /* parser wants more data than available */
  }
- return (b - (u_char *)buf);
+
+ bcopy(ber->br_rptr, buf, len);
+ ber->br_rptr += len;
+ ber->br_offs += len;
+
+ return len;
 }
 
 int
Index: usr.sbin/snmpd/ber.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/ber.c,v
retrieving revision 1.40
diff -u -p -r1.40 ber.c
--- usr.sbin/snmpd/ber.c 4 Jul 2018 15:21:24 -0000 1.40
+++ usr.sbin/snmpd/ber.c 6 Jul 2018 11:50:18 -0000
@@ -31,8 +31,6 @@
 
 #include "ber.h"
 
-#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
-
 #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
 #define BER_TYPE_SINGLE_MAX 30
 #define BER_TAG_MASK 0x1f
@@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
     int *cstruct);
 static ssize_t get_len(struct ber *b, ssize_t *len);
 static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
-static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
 static ssize_t ber_getc(struct ber *b, u_char *c);
 static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
 
@@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
  return totlen;
 }
 
-static ssize_t
-ber_readbuf(struct ber *b, void *buf, size_t nbytes)
-{
- size_t sz, len;
-
- if (b->br_rbuf == NULL)
- return -1;
-
- sz = b->br_rend - b->br_rptr;
- len = MINIMUM(nbytes, sz);
- if (len == 0) {
- errno = ECANCELED;
- return (-1); /* end of buffer and parser wants more data */
- }
-
- bcopy(b->br_rptr, buf, len);
- b->br_rptr += len;
- b->br_offs += len;
-
- return (len);
-}
-
 void
 ber_set_readbuf(struct ber *b, void *buf, size_t len)
 {
@@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
- return ber_readbuf(b, c, 1);
+ return ber_read(b, c, 1);
 }
 
 static ssize_t
 ber_read(struct ber *ber, void *buf, size_t len)
 {
- u_char *b = buf;
- ssize_t r, remain = len;
+ size_t sz;
 
- while (remain > 0) {
- r = ber_readbuf(ber, b, remain);
- if (r == -1)
- return -1;
- b += r;
- remain -= r;
+ if (ber->br_rbuf == NULL)
+ return -1;
+
+ sz = ber->br_rend - ber->br_rptr;
+ if (len > sz) {
+ errno = ECANCELED;
+ return -1; /* parser wants more data than available */
  }
- return (b - (u_char *)buf);
+
+ bcopy(ber->br_rptr, buf, len);
+ ber->br_rptr += len;
+ ber->br_offs += len;
+
+ return len;
 }
 
 int
Index: usr.sbin/ypldap/ber.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.23
diff -u -p -r1.23 ber.c
--- usr.sbin/ypldap/ber.c 4 Jul 2018 15:21:24 -0000 1.23
+++ usr.sbin/ypldap/ber.c 8 Jul 2018 13:34:13 -0000
@@ -31,8 +31,6 @@
 
 #include "ber.h"
 
-#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
-
 #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
 #define BER_TYPE_SINGLE_MAX 30
 #define BER_TAG_MASK 0x1f
@@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
     int *cstruct);
 static ssize_t get_len(struct ber *b, ssize_t *len);
 static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
-static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
 static ssize_t ber_getc(struct ber *b, u_char *c);
 static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
 
@@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
  return totlen;
 }
 
-static ssize_t
-ber_readbuf(struct ber *b, void *buf, size_t nbytes)
-{
- size_t sz, len;
-
- if (b->br_rbuf == NULL)
- return -1;
-
- sz = b->br_rend - b->br_rptr;
- len = MINIMUM(nbytes, sz);
- if (len == 0) {
- errno = ECANCELED;
- return (-1); /* end of buffer and parser wants more data */
- }
-
- bcopy(b->br_rptr, buf, len);
- b->br_rptr += len;
- b->br_offs += len;
-
- return (len);
-}
-
 void
 ber_set_readbuf(struct ber *b, void *buf, size_t len)
 {
@@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
- return ber_readbuf(b, c, 1);
+ return ber_read(b, c, 1);
 }
 
 static ssize_t
 ber_read(struct ber *ber, void *buf, size_t len)
 {
- u_char *b = buf;
- ssize_t r, remain = len;
+ size_t sz;
 
- while (remain > 0) {
- r = ber_readbuf(ber, b, remain);
- if (r == -1)
- return -1;
- b += r;
- remain -= r;
+ if (ber->br_rbuf == NULL)
+ return -1;
+
+ sz = ber->br_rend - ber->br_rptr;
+ if (len > sz) {
+ errno = ECANCELED;
+ return -1; /* parser wants more data than available */
  }
- return (b - (u_char *)buf);
+
+ bcopy(ber->br_rptr, buf, len);
+ ber->br_rptr += len;
+ ber->br_offs += len;
+
+ return len;
 }
 
 int

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: simplify ber_read()

Rob Pierce
On Sun, Jul 08, 2018 at 06:38:16PM +0200, Jeremie Courreges-Anglas wrote:

>
> When looking at the recent changes in this file I noticed that the
> presence of both ber_read() and ber_readbuf() was due to an incomplete
> cleanup from my part.
>
>   revision 1.32
>   date: 2018/02/08 18:02:06;  author: jca;  state: Exp;  lines: +6 -22;  commitid: YNQMco5lCS8YEifQ;
>   Kill ber.c support for direct fd read/writes
>
>   This mechanism is already unused and annotated with lots of XXX's, no
>   need to keep it around.  ok claudio@
>
> I think the recent changes would have been easier if the code had been
> further simplified.  Here's a shot at it:
> - stop looping in ber_read(), there is no fd read to handle any more so
>   calling ber_readbuf() once is enough
> - amend the condition which decides whether to return -1/ECANCELED:
>   - it makes little sense to pass a buffer length size of zero to
>   ber_read(), but then we should probably return 0 and not -1/ECANCELED
>   - I don't think we should perform partial reads: either the caller
>   function has the data it asked for, or nothing.  Allowing partial
>   reads means that we're putting the burden of handling an incomplete
>   buffer on the caller.
> - inline what remains of ber_readbuf() in ber_read()
>
> regress tests for ldapd and snmpd pass, it would be cool to have test
> reports from people who actually use those programs (ldap, ldapd, snmpd
> and ypldap).
>
> Looking for reviews and oks.  The diff is not that long but I can split
> it in smaller parts if it helps.

Thank you for this. Definitely would have make the recent changes a bit easier.

The code makes more sense to me now and works as expected.

ok rob@

> Index: usr.bin/ldap/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.bin/ldap/ber.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 ber.c
> --- usr.bin/ldap/ber.c 4 Jul 2018 15:21:24 -0000 1.11
> +++ usr.bin/ldap/ber.c 6 Jul 2018 11:50:24 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/ldapd/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/ldapd/ber.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 ber.c
> --- usr.sbin/ldapd/ber.c 4 Jul 2018 15:21:24 -0000 1.21
> +++ usr.sbin/ldapd/ber.c 6 Jul 2018 11:47:55 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/snmpd/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/snmpd/ber.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 ber.c
> --- usr.sbin/snmpd/ber.c 4 Jul 2018 15:21:24 -0000 1.40
> +++ usr.sbin/snmpd/ber.c 6 Jul 2018 11:50:18 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/ypldap/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/ypldap/ber.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 ber.c
> --- usr.sbin/ypldap/ber.c 4 Jul 2018 15:21:24 -0000 1.23
> +++ usr.sbin/ypldap/ber.c 8 Jul 2018 13:34:13 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

Reply | Threaded
Open this post in threaded view
|

Re: ber.c: simplify ber_read()

Claudio Jeker
In reply to this post by Jeremie Courreges-Anglas-2
On Sun, Jul 08, 2018 at 06:38:16PM +0200, Jeremie Courreges-Anglas wrote:

>
> When looking at the recent changes in this file I noticed that the
> presence of both ber_read() and ber_readbuf() was due to an incomplete
> cleanup from my part.
>
>   revision 1.32
>   date: 2018/02/08 18:02:06;  author: jca;  state: Exp;  lines: +6 -22;  commitid: YNQMco5lCS8YEifQ;
>   Kill ber.c support for direct fd read/writes
>
>   This mechanism is already unused and annotated with lots of XXX's, no
>   need to keep it around.  ok claudio@
>
> I think the recent changes would have been easier if the code had been
> further simplified.  Here's a shot at it:
> - stop looping in ber_read(), there is no fd read to handle any more so
>   calling ber_readbuf() once is enough
> - amend the condition which decides whether to return -1/ECANCELED:
>   - it makes little sense to pass a buffer length size of zero to
>   ber_read(), but then we should probably return 0 and not -1/ECANCELED
>   - I don't think we should perform partial reads: either the caller
>   function has the data it asked for, or nothing.  Allowing partial
>   reads means that we're putting the burden of handling an incomplete
>   buffer on the caller.
> - inline what remains of ber_readbuf() in ber_read()
>
> regress tests for ldapd and snmpd pass, it would be cool to have test
> reports from people who actually use those programs (ldap, ldapd, snmpd
> and ypldap).
>
> Looking for reviews and oks.  The diff is not that long but I can split
> it in smaller parts if it helps.
>

Looks good to me. OK claudio@
 

> Index: usr.bin/ldap/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.bin/ldap/ber.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 ber.c
> --- usr.bin/ldap/ber.c 4 Jul 2018 15:21:24 -0000 1.11
> +++ usr.bin/ldap/ber.c 6 Jul 2018 11:50:24 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/ldapd/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/ldapd/ber.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 ber.c
> --- usr.sbin/ldapd/ber.c 4 Jul 2018 15:21:24 -0000 1.21
> +++ usr.sbin/ldapd/ber.c 6 Jul 2018 11:47:55 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/snmpd/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/snmpd/ber.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 ber.c
> --- usr.sbin/snmpd/ber.c 4 Jul 2018 15:21:24 -0000 1.40
> +++ usr.sbin/snmpd/ber.c 6 Jul 2018 11:50:18 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
> Index: usr.sbin/ypldap/ber.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/ypldap/ber.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 ber.c
> --- usr.sbin/ypldap/ber.c 4 Jul 2018 15:21:24 -0000 1.23
> +++ usr.sbin/ypldap/ber.c 8 Jul 2018 13:34:13 -0000
> @@ -31,8 +31,6 @@
>  
>  #include "ber.h"
>  
> -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> -
>  #define BER_TYPE_CONSTRUCTED 0x20 /* otherwise primitive */
>  #define BER_TYPE_SINGLE_MAX 30
>  #define BER_TAG_MASK 0x1f
> @@ -48,7 +46,6 @@ static ssize_t get_id(struct ber *b, uns
>      int *cstruct);
>  static ssize_t get_len(struct ber *b, ssize_t *len);
>  static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm);
> -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes);
>  static ssize_t ber_getc(struct ber *b, u_char *c);
>  static ssize_t ber_read(struct ber *ber, void *buf, size_t len);
>  
> @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct
>   return totlen;
>  }
>  
> -static ssize_t
> -ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> -{
> - size_t sz, len;
> -
> - if (b->br_rbuf == NULL)
> - return -1;
> -
> - sz = b->br_rend - b->br_rptr;
> - len = MINIMUM(nbytes, sz);
> - if (len == 0) {
> - errno = ECANCELED;
> - return (-1); /* end of buffer and parser wants more data */
> - }
> -
> - bcopy(b->br_rptr, buf, len);
> - b->br_rptr += len;
> - b->br_offs += len;
> -
> - return (len);
> -}
> -
>  void
>  ber_set_readbuf(struct ber *b, void *buf, size_t len)
>  {
> @@ -1269,23 +1244,28 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
>  ber_read(struct ber *ber, void *buf, size_t len)
>  {
> - u_char *b = buf;
> - ssize_t r, remain = len;
> + size_t sz;
>  
> - while (remain > 0) {
> - r = ber_readbuf(ber, b, remain);
> - if (r == -1)
> - return -1;
> - b += r;
> - remain -= r;
> + if (ber->br_rbuf == NULL)
> + return -1;
> +
> + sz = ber->br_rend - ber->br_rptr;
> + if (len > sz) {
> + errno = ECANCELED;
> + return -1; /* parser wants more data than available */
>   }
> - return (b - (u_char *)buf);
> +
> + bcopy(ber->br_rptr, buf, len);
> + ber->br_rptr += len;
> + ber->br_offs += len;
> +
> + return len;
>  }
>  
>  int
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

--
:wq Claudio