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
>
