Howard Chu wrote:
Rich Megginson wrote:
> Any comments at all? Good? Bad? Don't Care?
Sorry, got busy with other things.
I have some concerns about struct ldifrecord, relatively minor though. In new
APIs I prefer to use struct bervals exclusively, no naked char *s.
(Ultimately, if we ever get to rewriting the C LDAP API, I would completely
eliminate char *s, as we already have in the server.)
What about the const char *errstr parameter to
ldap_parse_ldif_record()? That's only used for logging purposes.
Also, unless you're
actually using a BerVArray, you should use actual struct bervals, not pointers
to structs. I.e., unless there's some substantial benefit to having them
separately allocatable, it's better to avoid the extra malloc of the berval
structure and just embed the thing in the main structure. This is all standard
practice for OpenLDAP code.
In ldifutil.c your main comment refers to ldap_mod_from_ldifrec() but
no such function in the file.
The large #if 0 block /* we should faithfully encode the LDIF... should just
be deleted, we will never use that code in the future. I left it in place as a
reminder to myself, but when migrating code from place to place we should drop
anything obsolete instead of carrying it over forever.
> Rich Megginson wrote:
>> This patch provides low level LDIF support in libldif and high level
>> support in libldap.
>> The libldap support is in the form of
>> LDAP_F( int )
>> ldap_parse_ldif_record LDAP_P((
>> char *rbuf,
>> int linenum,
>> LDIFRecord *lr,
>> const char *errstr,
>> unsigned int flags,
>> void *ctx ));
>> Where rbuf and linenum come from ldif_read_record; lr is new (see
>> below); errstr is the string prefix to print for error/debug messages
>> (usually the program name e.g. "ldapmodify"), flags are one of these:
>> #define LDIF_DEFAULT_ADD 0x01 /* if changetype missing, assume
>> LDAP_ADD */
>> #define LDIF_ENTRIES_ONLY 0x02 /* ignore changetypes other than add */
>> #define LDIF_NO_CONTROLS 0x04 /* ignore control specifications */
>> and ctx is the memory context (for ber_*alloc_x)
>> LDIFRecord returns all of the data from the LDIF record, regardless of
>> what type of operation it is. The lr_op field is used to determine
>> the type of operation, and the other fields return the operation
>> specific data. There is also a section of several data fields used by
>> the internal implementation (lifted almost verbatim from
>> ldapmodify.c). This is cleaned up properly by ldap_ldif_record_done()
>> If the LDIF record in rbuf contains no actual record (e.g. the
>> version: field, comments only, etc.) then the return value of
>> ldap_parse_ldif_record() will be 0, but the lr_op field will also be 0.