[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: I-D update -- draft-ietf-ldapext-ldap-c-api-04.txt
Mark Smith writes:
> LDAPEXT participants: I believe this draft takes into account all of the
> comments made during the previous last call.
> (...) so we can progress this as soon as possible.
Not with me around to notice more problems:-)
No, seriously, I do want it progressed too, it might make more sense to
defer most of these to a draft released when rfc2251 & co are updated.
If any of these comments cause a significant delay, please pretend you
didn't see it.
========================================
Some bug fixes --
> 11.6. Searching
> timeout (...)
> NULL value for timeout causes the global default timeout
^^^^^^
Remove "global", that's the ldap_get_option(NULL,,) default. It should
be the session's default.
> stored in the LDAP session handle (set by using
^^^^^^^^^^^^^^^^^^^^^^^
> ldap_set_option() with the LDAP_OPT_TIMELIMIT parameter)
> LDAP_OPT_TIMELIMIT
> A limit on the number of seconds to spend on the search. A
> value of LDAP_NO_LIMIT (0) means no limit. Note that
unless the timeout parameter is NULL,
> the
> value from the session handle is ignored when using the
> ldap_search_ext() or ldap_search_ext_s() functions.
> 11.9. Comparing a Value Against an Entry
> The synchronous ldap_compare_ext_s() and ldap_compare_s() functions both
> return the result of the operation, either the constant LDAP_SUCCESS if
That should be LDAP_COMPARE_<TRUE/FALSE>.
> 17.5. Decoding
> ber_scanf() returns LBER_ERROR on error, and a different value on suc-
> cess.
Add: "If an error occurred, the values of all the output arguments are
undefined."
========================================
NULL return values --
> 11.4. Authenticating to the directory
> Parameters are:
Can dn, cred, passwd, serverctrls, or clientctrls be NULL?
Other NULLs may be missing elsewhere in the draft too.
E.g., maybe you should replace
serverctrls List of LDAP server controls.
clientctrls List of client controls.
with
serverctrls List of LDAP server controls, or NULL.
clientctrls List of client controls, or NULL.
everywhere in the draft.
> 14. Handling Errors and Parsing Results
> matcheddnp In the case of a return of LDAP_NO_SUCH_OBJECT, this result
I think *matcheddnp should be set to NULL if it was not set to anything
else.
> serverctrlsp This result parameter will be filled in with an allocated
Allow serverctrlsp == NULL in order to ignore this field.
> retoidp For extended results, this result parameter will be filled
> retdatap For extended results, this result parameter will be filled
Set these to NULL if they are absent in the message, as in section
11.14 (ldap_extended_operation*()).
Should the API set *referralsp and *servercredp to NULL if empty?
If an API error is returned, should all output arguments be set to NULL?
There might be other calls where error return should set / not set the
output parameters should be set/not set to NULL, or leave them undefined.
========================================
Error codes & texts --
> 10. LDAP Error Codes
>
> Many of the LDAP API routines return LDAP error codes, some of which
> indicate local errors and some of which are returned by servers.
Suggestion (may have been mentioned before):
Server result codes except LDAP_SUCCESS (0x01-0x50 below) MUST NOT be
used to report local API errors.
Otherwise we can't know if LDAP_OPT_<ERROR_STRING/MATCHED_DN> should be
printed along with ldap_err2string(code) after a synchronous operation.
(They come from server results, so they should not be printed after API
errors.) Assuming I guessed correctly _when_ these are set, of course.
To help that a bit, you could add
#define LDAP_IS_RESULTCODE(code) ((code) <= 0x50)
Returns nonzero if <code> is a valid server result code.
The <code> argument is evaluated exactly once, and MUST
be an error code returned from an LDAP call.
to be used as in my lderrout() example below.
> the LDAP error codes returned will be non-negative integers. Supported
> error codes are (hexadecimal values are given in parentheses after the
> constant):
>
> LDAP_SUCCESS (0x00)
-- server error codes
> LDAP_OPERATIONS_ERROR (0x01)
> (...)
> LDAP_OTHER (0x50)
-- local API errors
> LDAP_SERVER_DOWN (0x51)
> (...)
> 11.2. LDAP Session Handle Options
> LDAP_OPT_ERROR_NUMBER (0x31)
> (...)
> The code of the most recent LDAP error that occurred for this
> session.
Does that mean "most recent API error/server result code which was not
LDAP_SUCCESS"? How about LDAP_COMPARE_<TRUE/FALSE> (which are not
errors)?
Similarly, when are LDAP_OPT_<ERROR_STRING/MATCHED_DN> set (or reset)?
When are they NULL, and when are they ""? (See also my comments about
*matcheddnp in section 14.)
> 13. Obtaining Results and Peeking Inside LDAP Messages
Suggestion (may have been mentioned before):
If ldap_result() returns -1, it MUST NOT consume any LDAP message
received from the server. If an API error (e.g. out of memory)
happens while retrieving a valid LDAP message, the API SHOULD return
an LDAPMessage with correct message id, type and result code, and defer
the error to calls that _use_ the LDAPMessage.
Otherwise, if ldap_result() discards the message and returns an error,
the client will think the message has not been received, and keep
waiting for it.
Of course, the above may be difficult at times, so:
The API MAY also abort the connection at need. On such an aborted
connection, the API MAY discard the last received Ldap Messages at
need, so ldap_result() can return failure. An aborted connection
(from which messages have been discarded or rejected) cannot be
reopened. (Otherwise applications would see loss of messages in
the middle of a session.)
> 14. Handling Errors and Parsing Results
> matcheddnp In the case of a return of LDAP_NO_SUCH_OBJECT, this result
> parameter will be filled in with a DN indicating how much
or with return of LDAP_ALIAS_PROBLEM, LDAP_INVALID_DN_SYNTAX, or
LDAP_ALIAS_DEREF_PROBLEM (according to rfc2251 4.1.10). Or maybe you
should just remove the special case and set it if the error code is a
valid server result code, and trust the server to do the right thing.
(i.e. set it if the error code != LDAP_SUCCESS and my suggested macro
LDAP_IS_RESULTCODE(error code) would return true.)
BTW, say if it should be "" or NULL when empty.
(Maybe "" when one of the 4 codes above, and NULL otherwise:-)
About detecting errors:
For some calls, NULL is indistinguishable from error. A possible fix
for the ones that take an LDAP* parameter could be to always set
LDAP_OPT_ERROR_NUMBER, or to always set it when it returns NULL.
That's not quite backwards compatible, though.
These calls include:
ldap_<first/next>_<message/reference/entry/attribute>
ldap_get_<values/values_len>
Maybe also
ldap_get_dn, ldap_dn2ufn, ldap_explode_dn.
if someone mistakenly implement these to return NULL for the root DN.
BTW, you could add a note in section 16.4 that ldap_get_dn, ldap_dn2ufn,
and ldap_explode_dn do not return NULL for the root DN, but an empty
string or array (containing only the terminating \0 or NULL element).
========================================
Unclear text --
> 11.2. LDAP Session Handle Options
> LDAP_OPT_HOST_NAME (0x30)
When ldap_init() is creating the LDAP_OPT_HOST_NAME value and 2nd arg !=
389, I'm not sure whether or not it should add ":portno" to hostnames
with no portno component. (I'm fairly sure think I mentioned this
before, but can't the reply.)
> 13. Obtaining Results and Peeking Inside LDAP Messages
> Once a chain of messages has been returned to the caller, it is no
> longer tied in any caller-visible way to the LDAP request that produced
> it.
Nor to the LDAP* session handle? (I may have said that before)
> Therefore, a chain of messages returned by calling ldap_result() or
> by calling a synchronous search routine will never be affected by subse-
> quent LDAP API calls (except for ldap_msgfree() which is used to (...)
If the message *is* tied to the LDAP* as well, add:
"and ldap_unbind(), and possible API extensions". (Like a session
option to set the character set with which the message is decoded.)
========================================
Declarations --
> 6. Header Requirements
> Use of the 'const' Keyword
Might allow the implementation to use more qualifiers: More 'const', or
'restrict' in the next C standard (C9X).
> 11.10. Modifying an entry
>
> The following routines are used to modify an existing LDAP entry. There
> are four variations:
>
> typedef struct ldapmod {
> int mod_op;
> char *mod_type;
> union mod_vals_u {
This adds `mod_' to struct/union tag namespace (appendix B).
I suggest you call it `union ldapmod_vals_u' instead.
Also, you may wish to move the union declaration out of the struct.
Otherwise the union has a different name in C and C++, so a mixed C/C++
declaration of an `ldapmod_vals_u' toplevel variable must be coded
something like this (untested):
#ifdef __cplusplus
extern union LDAPMod::ldapmod_vals_u var;
#else
extern union ldapmod_vals_u var;
#endif
========================================
Enhancements --
> 11.2. LDAP Session Handle Options
> LDAP_OPT_ERROR_STRING (0x32)
> LDAP_OPT_MATCHED_DN (0x33)
It's impossible to access referrals and controls returned from the
server to synchronous operations; maybe you'll want to add new options
for these as well. However, maybe this addition to ldap_parse_result()
would be cleaner:
res (...) If res == NULL, the result from the last operation is
used, which MUST have been synchronous (ldap*_s() or _st())
and have returned a server result code.
This SHOULD only be done immediately after the synchronous
call, because the API MAY override the returned values with
(server or API) errors from later API calls, or from
LDAP_OPT_ERROR_NUMBER, LDAP_OPT_ERROR_STRING, or
LDAP_OPT_MATCHED_DN.
That requires the API to save this status for _s calls, but imposes no
such overhead for asynch calls.
Another suggestion:
LDAP_OPT_SESSION_TIMEOUT
Type for invalue parameter: int *
Type for outvalue parameter: int *
Description:
An advisory timeout in seconds from the last activity
(which the API is aware of) on the connection. When the
timeout expires, the API MAY abort the connection. The
default value is implementation-defined. LDAP_NO_LIMIT (0)
means the maximum timeout supported by the implementation;
maybe no limit.
> 11.6. Searching
> sizelimit For the ldap_search_ext() and ldap_search_ext_s() calls,
> this is a limit on the number of entries to return from the
> search. A value of LDAP_NO_LIMIT (0) means no limit.
Suggestion: Use the session's default sizelimit if sizelimit == -1.
(Also modify the comment below about LDAP_OPT_SIZELIMIT accordingly.)
> 17.3. Encoding
> 28.1. API Changes
> "Encoded ASN.1 Value Manipulation" section: (...)
> Changed the format specif-
> ier for Bitstring in ber_printf() from 'X' to 'B' to match
> ber_scanf().
Since that 'X' option is old by now, you might add a note in the
description of ber_printf (17.3) that 'X' has also been used, and/or
allow both 'X' and 'B'.
========================================
Unportable architecture dependence --
> 11.2. LDAP Session Handle Options
> #define LDAP_OPT_ON ((void *)1)
I have mentioned some or all of this before, but don't remember what:-(
((void*)1) is may dump core, or be == ((void*)0), or... anything. I
suggest an implementation-defined non-NULL pointer (or pointer constant)
to void. If you don't require it to be a pointer constant, we can use
portable C instead of implementation-defined tricks like ((void*)1):
extern char ldap_opt_on[]; /* never dereferenced */
#define LDAP_OPT_ON ((void *)ldap_opt_on)
> 11.3.1. A Client Control That Governs Referral Processing
> the ldctl_value field MUST be set to a
> 4-octet value that contains a set of flags. The ldctl_value.bv_len
> field MUST always be set to 4. The ldctl_value.bv_val field MUST point
> to a 4-octet integer flags value.
This assumes that a byte (char) is 8-bit (== an octet) and that there is
a 4-byte integer type, but even then we must use #ifdef to find _which_
integer type is 4-byte. I don't know why you want a 4-octet value,
but maybe you mean an ber_uint_t?
"the ldctl_value field MUST contain a ber_uint_t that contains a set of
flags: The ldctl_value.bv_len field MUST be set to sizeof(ber_uint_t),
and the ldctl_value.bv_val field MUST point to a ber_uint_t which
contains the flags value."
Also move the definition of ber_uint_t up from section 17.1.
Or if you want 4 octets with the the value part of the BER encoding of
an integer, it would be easier to use that directly:
#define LDAP_CHASE_SUBORDINATE_REFERRALS "\0\0\0\40"
#define LDAP_CHASE_EXTERNAL_REFERRALS "\0\0\0\100"
#define LDAP_CHASE_ALL_REFERRALS "\0\0\0\140"
#define LDAP_CHASE_NO_REFERRALS "\0\0\0\0"
...or allow a mere 1-octet value, so we can OR them together.
> 17.1. BER Data Structures and Types
> width of `ber_slen_t' MUST be at least 32 and no larger than that of
> `unsigned long'.
^^^^^^^^^^^^^^ I think that's `long', not `unsigned long'.
This makes a difference if we have:
ber_slen_t = long with width 64,
ber_len_t = unsigned long with width 63 + padding bit (the sign bit).
Though you could just as well to drop most of the description ber_slen_t
(including that of the width) and just say
The `ber_slen_t' type is the signed variant of the `ber_len_t'
integral type. (I.e. if `ber_len_t' is unsigned long, then
`ber_slen_t' is signed long.) The `impl_slen_t' in the `ber_len_t'
typedef MUST be replaced with an appropriate type.
Similarly, if `int' has 32 bits, but `unsigned int' has 31 bits + 1
padding bit, then these sentences:
The `ber_int_t' and `ber_uint_t' types are the signed and unsigned vari-
ants of an integral type (...)
The width (number of significant bits) of `ber_uint_t' MUST be at
least 32 and no larger than that of `unsigned long'.
mean that `ber_int_t' and `ber_uint_t' must be `long' and `unsigned
long', even though `int' is wide enough for a ber_int_t.
So, you can
1. drop the sentence about the width of ber_uint_t or replace it with
"it is RECOMMENDED that `ber_uint_t' is at least 32 bits wide",
and thus allow that `(ber_uint_t)negative_value' loses information,
2. or drop the currently misleading sentence about width of `ber_int_t',
since it inherits the width requirement of `ber_uint_t' in any case,
3. or rewrite 2st sentence so they need not be corresponding types
(e.g. ber_int_t = int, ber_uint_t = unsigned long),
I thought #1 was the idea, and I hope nobody votes for #3:-)
BTW, it would be a bit kinder to the host above (if `unsigned int' is 31
bits + 1 padding bit) if we didn't require `ber_len_t' to be at least 32
bits, but only required that of `ber_slen_t'. That would mean
`ber_len_t' would be at least 31 bits, and it could fit in an unsigned
int instead of having to use unsigned long.
> 17.3. Encoding
> 'B' Bitstring.
Could add a note that some hosts may have more than 8 bits per byte,
and I also think it should be 'unsigned char' so there won't be any sign
extension or overflow trap when the user constructs the bitstring. Also
that's better for char which is not two's complement, though that
probably doesn't work anyway.
And refer to that note in 'B' in 17.5.
> 17.5. Decoding
>
> The following two macros are available to applications: LBER_ERROR and
> LBER_DEFAULT. Both of these macros MUST be #define'd as integer con-
> stants that are both compatible with the ber_tag_t type and for which
> all bits have the value one. For example, ISO C guarantees that these
> definitions will work:
>
> #define LBER_ERROR ((ber_tag_t)-1)
> #define LBER_DEFAULT ((ber_tag_t)-1)
>
> The intent is that LBER_ERROR and LBER_DEFAULT are both defined as the
> integer value that has all bits set to 1, as such a value is not a valid
> BER tag.
That intent makes assumptions about the implementation's mapping of
ber_tag_t <-> BER-encoded (tag value, class, constructed-bit).
You could simply drop the talk about bits, and instead say that
LBER_ERROR and LBER_DEFAULT MUST be invalid tags in the implementation's
mapping of tag <-> ber_tag_t.
E.g. if the host represents ber_tag_t == long as two 32-bit words where
the sign bit is ignored, the 62-bit (ber_tag_t)-1 might have the BER
encoding (FF FF FF FF FF FF FF 3F), a valid tag. That's if it is
read using ((byte & mask) << bits) operations.
BTW, if you do want 0xFF octets, *say* octets and not bits. If the host
has 16-bit char and ber_tag_t is read through an array of unsigned char,
each "octet" in ((ber_tag_t)-1) gets the value 0xFFFF. Which is
certainly an invalid octet, so it fulfills the intent of invalid tags.
========================================
Editorial comments / minor clarifications --
> 5. Overview of LDAP API Use and General Requirements
> (...)
> Operations can be performed either synchronously or asynchronously.
Add a note that the "asynchronous" routines are slightly misnamed: They
can block until the operation can be sent or buffered by the system.
> 6. Header Requirements
> Name and Inclusion
> Applications only need to include a single header named ldap.h
> to access all of the API services described in this document.
> Therefore, the following C source program MUST compile
and run
> without errors
> 7. Common Data Structures and Types
> typedef struct ldapmsg LDAPMessage;
>
/* typedef LDAPControl: See section 11.3 */
/* typedef LDAPMod: See section 11.10 */
> The LDAP structure is an opaque data type that represents an LDAP ses-
> sion Typically this
^^^
Missing "."
> 11. Performing LDAP Operations
> This section describes each LDAP operation API call in detail. All func-
^^^
*Most*, not *all*. (Not ldap_init/ldap_open.)
> tions take a "session handle,"
> 13. Obtaining Results and Peeking Inside LDAP Messages
> ldap_msgtype() returns the type of an LDAP message.
(add a line break here)
> ldap_msgid()
> returns the message ID of an LDAP message.
> 14. Handling Errors and Parsing Results
> ldap_err2string() (...) returns a pointer to static data.
Please add 'it MUST NOT return NULL' even though that's implied by the
above text, to emphasize that the result is always printable.
> 17.1. BER Data Structures and Types
> typedef impl_unit_t ber_uint_t; /* unsigned equivalent of ber_int_t */
Typo: ^^^^ fix unit -> uint
> 17.3. Encoding
> int ber_printf( BerElement *ber, const char *fmt, ... )
Missing ';' after ')'
> 17.4. Encoding Example
> int encode_example1(const char *s, ber_int_t val1, ber_int_t val2,
> struct berval **bvPtr)
> {
> BerElement *ber;
> int rc = -1;
*bvPtr = NULL; /* in case of error */
> 17.6. Decoding Example
> for (i = 0; attrs != NULL && attrs[i] != NULL; i++) {
> /* *** use attrs[i] */
> ldap_memfree(attrs[i]);
> }
> ldap_memfree(attrs);
Should be ldap_memfree((char *)attrs);
> 24. Appendix B - Namespace Consumed By This Specification
> The following 6 prefixes are used in this specification to name struc-
> tures, unions, and typedefs:
> ldap
> LDAP
> PLDAP
Remove PLDAP, change "6 prefixes" -> 5.
> 26.9. Deprecated Functions
> ldap_perror()
> Use ldap_err2string() instead.
Could replace that with
Use fprintf( stderr, ... ldap_err2string() ... ) instead.
because people keep printing errors to stdout. Duh.
> 27. Appendix E - Data Types and Legacy Implementations
> Similar code can be used to define appropriate ber_len_t and ber_int_t
> types.
and ber_slen_t and ber_uint_t.
> 28.2. Editorial Changes
> Types: Modified implementation-specific typedefs to use `impl_XXX_t'
> convention.
I'd suggest `<impl_XXX_t>' (i.e. `typedef <impl_len_t> ber_len_t'), so a
reader who just scans the document sees that there is no type called
`impl_len_t'.
Indentation:
> 11.1. Initializing an LDAP Session
Indent the ldap_init/ldap_open declarations (including parameters) by 3
spaces, then all functions in the draft get the same indentation.
> 11.5. Closing the session
> int ldap_unbind_ext( LDAP *ld, LDAPControl **serverctrls,
> LDAPControl **clientctrls );
> 17.5. Decoding
> ber_tag_t ber_peek_tag( BerElement *ber,
> ber_len_t *lenPtr );
Indent 2nd line in these declarations by 4 spaces, then they'll be
indented like the other parameters in the draft.
========================================
Horror --
Proper error checking in the example. Overly proper, you'll likely want
to drop at least the lines preceded with '?'.
> 23. Appendix A - Sample C LDAP API Code
>
> #include <stdio.h>
> #include <ldap.h>
### Replace LDAP_IS_RESULTCODE( rc ) with rc <= 0x50
### if you do not add my suggested LDAP_IS_RESULTCODE to the draft:
void lderrout( const char *msg, int rc, LDAP *ld )
{
char *errstr = NULL, *matched = NULL;
if ( rc == -1 )
ldap_get_option( ld, LDAP_OPT_ERROR_NUMBER, &rc );
if ( ld && rc != LDAP_SUCCESS && LDAP_IS_RESULTCODE( rc ) ) {
### What to do when here
### depends on the answers to when the API updates these values.
ldap_get_option( ld, LDAP_OPT_ERROR_STRING, &errstr );
ldap_get_option( ld, LDAP_OPT_MATCHED_DN, &matched );
}
fprintf( stderr,
(errstr && *errstr) ? "%s: %s; %s\n" : "%s: %s\n",
msg, ldap_err2string( rc ), errstr );
if ( matched && *matched )
fprintf( stderr, "\t(matched: %s)\n", matched );
if ( matched )
ldap_memfree( matched );
if ( errstr )
ldap_memfree( errstr );
}
int
> main()
> {
> (...)
> char **vals;
?## Needed (along with the ldap_set_option() calls) unless you modify
?## the draft to say routines that return a possible error code (NULL)
?## will set LDAP_OPT_ERROR_NUMBER to LDAP_SUCCESS at success.
? const int success = LDAP_SUCCESS;
> if ( (ld = ldap_init( "dotted.host.name", LDAP_PORT )) == NULL )
{
fputs( "Could not open LDAP session\n", stderr );
> return 1;
}
> fprintf( stderr, "ldap_simple_bind_s: %s\n",
> ldap_err2string( rc ));
> (...)
> fprintf( stderr, "ldap_search_s: %s\n",
> ldap_err2string( rc ));
Replace these with:
lderrout( "LDAP-bind", rc, ld );
lderrout( "LDAP-search", rc, ld );
> /* step through each entry returned */
? ldap_set_option( ld, LDAP_OPT_ERROR_NUMBER, &success);
> for ( e = ldap_first_entry( ld, res ); e != NULL;
> e = ldap_next_entry( ld, e ) ) {
> /* print its name */
> dn = ldap_get_dn( ld, e );
if ( dn == NULL ) {
lderrout( "Error in DN", -1, NULL );
? ldap_set_option( ld, LDAP_OPT_ERROR_NUMBER, &success);
break; /* try next entry */
}
> printf( "dn: %s\n", dn );
> (...)
> /* print each attribute */
> for ( a = ldap_first_attribute( ld, e, &ptr ); ... ) {
>
> /* print each value */
> vals = ldap_get_values( ld, e, a );
if ( vals == NULL ) {
lderrout( "Error in values", -1, NULL );
? ldap_set_option( ld, LDAP_OPT_ERROR_NUMBER, &success);
} else {
> for ( i = 0; vals[i] != NULL; i++ ) {
> printf( "\t\tvalue: %s\n", vals[i] );
> }
> ldap_value_free( vals );
}
> ldap_memfree( a );
> }
? if ( ldap_get_option( ld, LDAP_OPT_ERROR_NUMBER, &rc )
? == LDAP_SUCCESS && rc != LDAP_SUCCESS )
? lderrout( "Error in attributes", rc, ld );
? ldap_set_option( ld, LDAP_OPT_ERROR_NUMBER, &success );
> if ( ptr != NULL ) {
> ber_free( ptr, 0 );
> }
>
>
> }
? if ( ldap_get_option( ld, LDAP_OPT_ERROR_NUMBER, &rc )
? == LDAP_SUCCESS && rc != LDAP_SUCCESS )
? lderrout( "Could not get entry", rc, ld );
? ldap_set_option( ld, LDAP_OPT_ERROR_NUMBER, &success );
> /* free the search results */
--
Hallvard