[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#4314) C API for paged results



On Sun, 2006-01-08 at 15:58 +0000, hans@it.vu.nl wrote:

> OpenLDAP does not provide any C API calls to encode/parse paged results
> requests/responses (RFC2696), although it supports the page control in
> the server. Augmenting the C API with such calls would fix this imbalance
> and also make porting certain client software to OpenLDAP easier.
> 
> So here's a proposed a patch that implements the ldap_create_page_control
> and ldap_parse_page_control APIs:
> 
>     http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch

Hans,

thanks for the contribution.  I have essentially one comment, which
applies to your contribution as well as to many other implementations of
controls spread into OpenLDAP client library code (and, I guess, in
other implementations of the client library).

The "tough" part of creating or parsing a control is encoding/decoding
the control value, if any; what we really need is hiding the details of
value encoding/decoding behind a clear interface that takes/returns the
high level info related to the control (the same applies to extended
operations).

The API you propose appears to be based on that of the vlv control,
which is unnecessarily "pedantic" and wastes a lot of memory in
unnecessary memory allocation both ways.  The typical use of server
controls is with the ldap_*_ext* functions; in my practice I usually
write code like:

    LDAPControl c, *ctrls[2];
    
    c.ldctl_oid = LDAP_CONTROL_*;
    c.ldctl_iscritical = iscritical;
    c.ldctl_value = /* the tough part */ ;

    ctrls[0] = &c;
    ctrls[1] = NULL;

    ldap_*_ext(ld, ..., ctrls, ...);

Similarly, when parsing results:

    ldap_parse_result(ld, msg, ..., &ctrls, freeme);
    if (ctrls != NULL) {
        for (i = 0; ctrls[i] != NULL; i++) {
            if (strcmp(ctrls[i]->ldctl_oid, LDAP_CONTROL_*) == 0) {
                handle( &ctrls[i]->ldctl_value ); /* the tough part */
            } else {
                /* ??? */
            }
        }
    }

So I think your API is fine for consistency with other controls, but I'd
prefer to split it in two levels:

"value" level: deal with encoding/decoding the value; something like

int
ldap_create_page_control_value(
        int pagesize,               /* in */
        struct berval *cookie,      /* in */
        struct berval *value );     /* out; only malloc() bv_val */

int
ldap_parse_page_control_value(
        struct berval *value,      /* in */
        int *estimate,             /* out */
        struct berval *cookie);    /* out; only malloc() bv_val */

These two functions can be used by "smart" code to eliminate the
redundant part (OID and LDAPControl allocation in create; OID lookup in
parse; and so on).  The calls you propose could still be present for
consistency with the current API; they can use the "smart" calls to
handle the ldctl_value.

On a related note, the cookie is supposed to be __empty__ as per RFC
2696 either when sent the first time or when returned the last time, so
I don't see any reason to have ldap_parse_page_control() pass a "struct
berval **" for it, since that implies that also the "struct berval" has
to be malloc()'ed; simply pass a "struct berval *" and make the function
set bv_len = 0 when the cookie is empty.

Comments?

p.




Ing. Pierangelo Masarati
Responsabile Open Solution
OpenLDAP Core Team

SysNet s.n.c.
Via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
------------------------------------------
Office:   +39.02.23998309          
Mobile:   +39.333.4963172
Email:    pierangelo.masarati@sys-net.it
------------------------------------------