[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: I-D update -- draft-ietf-ldapext-ldap-c-api-04.txt
Hallvard B Furuseth wrote:
>
> 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.
I'd like to minimize any changes to the draft at this point. But valid
comments should not be ignored either, so I have responded in detail
below to each of your comments and suggested a course of action. My
general philosophy is that at this point in time clarifications are good
to adopt, but substantial changes are not. As always, thank you very
much for your detailed comments.
>
> ========================================
>
> 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.
Agreed. I think this small change should be made.
>
> > 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,
Right. This is explained fully in the text that describes the "timeout"
parameter. So I don't think the text needs to be altered. It might be
clearer to remove the descriptions of LDAP_OPT_TIMELIMIT,
LDAP_OPT_SIZELIMIT, and LDAP_OPT_DEREF from section 11.6 because they
are already described in section 11.2.
> > 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>.
Correct. I think this should be fixed because the text is misleading
and confusing.
>
> > 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."
That sounds like a good addition to me.
>
> ========================================
>
> 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.
Yes they definitely can be NULL if no controls are to be used. I like
your suggested change. The same change needs to be made in sections
11.4, 11.5, 11.6, 11.9, 11.10, 11.11, 11.12, 11.13, 11.14, 12, and 14.
>
> > 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.
Agreed. This is a necessary clarification.
>
> > serverctrlsp This result parameter will be filled in with an allocated
>
> Allow serverctrlsp == NULL in order to ignore this field.
Agreed. This omission is just an oversight. For the similar parameter
in sections 16.5 and 16.6 this additional text appears: "If serverctrlsp
is NULL, no controls are returned."
>
> > 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*()).
Agreed, this should be required for consistency.
>
> Should the API set *referralsp and *servercredp to NULL if empty?
I think so, yes.
>
> If an API error is returned, should all output arguments be set to NULL?
I think it is okay to say that the values are undefined. Other opinions
are welcome.
>
> 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.
There are a few more:
msgidp in many places (e.g., ldap_search_ext()).
servercredp for ldap_sasl_bind_s().
errcodep, errmsgp, and serverctrlsp for ldap_parse_result().
serverctrlsp for ldap_get_entry_controls()
referralsp and serverctrlsp for ldap_parse_reference()
I think the values should be described as "undefined" if an API error
occurs (e.g., if ldap_parse_result() fails) but we should require that
they be set to NULL if they are absent from the LDAP result or if there
is no simple way to tell if an API error occurred or if the server
returned an error (e.g., for ldap_sasl_bind_s()). I could also be
convinced that these parameters should be set to NULL if any API errors
occur.
>
> ========================================
>
> 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)
> > (...)
Your proposal is controversial to me. I don't think we should restrict
API implementors so that they can't generate a result code themselves
that looks like a "server" error code. I believe that implementations
should clear (or set as appropriate) the matched DN and error text
values whenever they set the error number.
>
> > 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)?
You make a good point -- "error" is perhaps the wrong term. It is a
result code, really. LDAP_COMPARE_TRUE/FALSE are the only non-errors
other than LDAP_SUCCESS I think. We should clarify the text as you
suggest.
>
> 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.)
See my comment above. This should be clarified. NULL and "" are fairly
similar, although in the implementation I have worked on most recently
NULL means "no value was available at all" and "" (a zero length string)
means "a value was returned, e.g., in an LDAPResult protocol message,
but it was zero-length string."
>
> > 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.)
But what should the behavior be after a connection is aborted? Does
that mean that all calls to ldap_result() will return -1 and so will
synchronous operation calls? What error code is returned?
LDAP_SERVER_DOWN? I am not sure we should try to add this concept now
because I think a lot of explanation is needed. I agree that specifying
this would help application writers though.
>
> > 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:-)
I agree that we should just remove the reference to the noSuchObject
error code. I think empty (a zero-length string was returned) is
different than NULL (no value available) and that the API implementation
should return whatever the server sent or NULL if the server sent no
value at all. Since all LDAPResult messages include matchedDN and
errorMessage as required fields, whenever an LDAPResult is processed the
value of *matcheddnp will not be NULL.
>
> 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.
I agree that LDAP_OPT_ERROR_NUMBER should always be set for the calls
that take an LDAP *. For the calls that don't, there is simply no way
to detect failure. Proposed solutions include Kurt's ldap_errno
extension proposal (draft-zeilenga-ldap-c-api-errno-00.txt) and the
LDERRNO extension I just published
(draft-smith-ldap-c-api-ext-lderrno-00.txt). I think we should tackle
the inadequate error reporting in an extension (as a separate document).
>
> 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).
By the root DN do you mean "" or NULL? Our implementation tends to
treat NULL as a synonym for "". I agree that we should add a note like
you suggest to section 16.4 as this is a good clarification.
>
> ========================================
>
> 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.)
I say yes, the :portno should be added since it might be useful to the
client application. But I don't much care which way we go -- but we
should specify.
>
> > 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)
Yes, this was discussed before. To be honest, I don't remember the
outcome. I think it is safer and less restrictive to NOT require
implementations to ensure that the message chains are independent of the
session handle. In the U-M and Netscape implementations (and probably
OpenLDAP as well), the messages are not tied to the session handle. But
I could imagine that it might be more convenient of efficient for an API
implementation to do so. Other opinions?
>
> > 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.)
Right, I think we should add this text.
>
> ========================================
>
> 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).
I would rather not loosen this up right now.
>
> > 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.
Well, we already use mod_ in a macro so I don't think it matters too
much (but if we do not change this as you suggest case Appendix B should
be updated).
>
> 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
Fine by me. C++ strikes again!
>
> ========================================
>
> 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.
I don't like this. Creeping featurism. If you really need access to
the referrals and controls, use the asynchronous calls plus ldap_result
and ldap_parse_result(). But you could proposed a C API extension for
this ;-)
>
> 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.
Again, this is a good candidate for a C API extension.
> > 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.)
I support this change because I think we have removed some useful
functionality by not providing an easy way for the caller to use the
session sizelimit.
>
> > 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'.
Okay, good idea. I'd like to add a note so that future editors or
authors of extensions do not reuse 'X'.
>
> ========================================
>
> 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)
Is it really true that (void *)1 might dump core? And that (void *)0
might be == to (void *)1? Yuck. I have never run into that, but I
defer to the C language experts out there. I have no problem with
changing the text to say something like:
#define LDAP_OPT_ON <impl_void*_t>
#define LDAP_OPT_OFF ((void *)0)
LDAP_OPT_ON MUST be defined as a non-zero pointer to void;
that is, <impl_void*_t> MUST be replaced with a non-zero
pointer to void.
>
> > 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?
I didn't write this section, but I agree it could be improved a bit (no
pun intended). Perhaps the struct berval definition should be changed
to say that bv_len is "Length of data in octets"? Maybe this is a can
of worms.
>
> "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.
This suggestion seems like the simplest solution to me.
>
> 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.
I don't have strong feelings about this, but the implementation I know
about that supports LDAP_CONTROL_REFERRALS likely uses a 32-bit quantity
for the flags, so I suggest we go with the ber_uint_t solution. If we
do that I'll need to remove the statement in section 17.1 that says
"Note that the `ber_uint_t' type is not used directly in the C LDAP
API...."
>
> > 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'.
Right, good catch.
>
> 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.
I really like this suggestion. I am not very happy with the complex
wording that is there now.
>
> 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:-)
My intention (based on your comments last time I think) was #1. But #2
seems simpler and is closest in spirit to what everyone will implement
anyway.
>
> 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.
Do you know of any such hosts? I don't feel strongly about this. I
suspect 31 or 32 bits are enough for our use of ber_len_t.
>
> > 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.
But we use char * in struct berval anyway. Is it better to be
consistent here or to use unsigned char * here?
>
> > 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.
I'd rather specify the value if possible, but I realize that ber_tag_t
is already implementation specific. Still, one could imagine writing a
follow-on document that describes a binary-compatible C LDAP API (by
restricting the size and types for all of the things that we don't in
this core API document).
>
> 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.
I think we should say octets instead, e.g.:
LBER_ERROR and LBER_DEFAULT MUST be defined
as ber_tag_t integral values that are treated as
invalid tags by the API implementation. It is
RECOMMENDED that the values of LBER_ERROR and
LBER_DEFAULT be the same and that they be defined
as values where all octets have the value 'FF'.
>
> ========================================
>
> 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.
Well, I wouldn't say they are misnamed. But we don't define
"asynchronous." Do other people think we should?
>
> > 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
Okay, I suppose it is good to add "and run."
>
> > 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 "."
Good catch. Definitely we should fix all typos!
>
> > 11. Performing LDAP Operations
>
> > This section describes each LDAP operation API call in detail. All func-
> ^^^
>
> *Most*, not *all*. (Not ldap_init/ldap_open.)
Good catch again.
>
> > 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.
Okay.
>
> > 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.
Yes, I'd like to add this. I made a comment to this same effect in my
LDERRNO extension draft. Any objections?
>
> > 17.1. BER Data Structures and Types
>
> > typedef impl_unit_t ber_uint_t; /* unsigned equivalent of ber_int_t */
>
> Typo: ^^^^ fix unit -> uint
Good catch again.
>
> > 17.3. Encoding
>
> > int ber_printf( BerElement *ber, const char *fmt, ... )
>
> Missing ';' after ')'
Right.
>
> > 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 */
This is good programming practice, but it is not essential given that
encode_example1() does return an error indication.
>
> > 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);
True.
>
> > 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.
Good catch again.
>
> > 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.
Okay, but those people are not very sharp!
>
> > 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.
Right, I forgot to add those two.
>
> > 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'.
Good idea. Kurt Z. made the original suggestion to use impl_len_t and
similar constructs. Kurt, any comments? Of course everyone should read
the entire draft ;-)
>
> 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.
Okay, no problem.
>
> > 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.
It is amazing to me that the indentation is as consistent as it is...
the source for the draft is a huge ms(5) (nroff) document. I'll fix
this before we go to RFC.
>
> ========================================
>
> 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
Let me take a bit more time to absorb your proposed error checking
changes. I need to think about the changes in light of our discussion
about when the error number, matched DN, and error message information
is updated. I think it is a reasonable idea to break the error
reporting out into a function, but maybe that makes the example less
clear to some people? Maybe not.
That's all for now. Whew!
--
Mark Smith
iPlanet Directory Architect / Sun-Netscape Alliance
My words are my own, not my employer's. Got LDAP?