h.b.furuseth@usit.uio.no wrote:
Error handling:
ldif_tool_entry_first() cannot return NOID.
Dunno (untouched)
ldif_back_<add,modify,delete,modrdn> always return success (0). Maybe some of this is a requirement of back-config? If so I suggest that back-config sets a ldif_info.li_optimism flag and that back-ldif otherwise treats errors as errors.
Don't think so: now they return rs->sr_err and everything keeps working.
If rmdir() in ldif_back_delete() fails with errno != ENOENT, rs->sr_err is set to LDAP_NOT_ALLOWED_ON_NONLEAF - but is then immediately overwritten with LDAP_UNWILLING_TO_PERFORM.
Fixed
ldif_tool_entry_put() sets 'res' to various LDAP result codes, but only uses it as a boolean. Are the codes supposed to be logged?
Dunno (untouched)
Files:
back-ldif overwrites files instead writing to a temp file and renaming when complete. The current way corrupts entries if a write fails halfway through, e.g. due to a full disk. Also the file can be left truncated if internal slapd operations fail. If the OS supports it, back-ldif should make a new file in the same directory and rename to the old filename when complete.
Fixed; now mkstemp(3) is used, and rename(2) only in case of success.
Locks:
The global entry2str_mutex is locked around spew_entry() instead of inside it, so it spans more file operations than necessary. Should be held more briefly, I think. Though it does make kind of sense now: It is locked before the file is truncated so that the file won't be in truncated state for long while slapd waits for the mutex. But that's not quite how I'd go about solving that problem; see above. (I have not checked if the mutex doubles as a mutex to synchronize internal back-ldif operations. So I don't know if lock/unlock can just be moved inside spew_entry().)
Moved as close as possible to entry2str() whenever not used also to serialize backend-specific operations (like in ldif_back_modrdn)
struct bvlist quirks:
.num holds an unnecessarily duplicated string. It only needs to a bechar, holding the character being overwritten with a '\0'. (And replace the AC_MEMCPY in r_enum_tree() which restores it.)
For that matter, .num is not needed at all if the '\0' can replace the IX_FSL (usually '{') instead of the character following it. (And decrement .off with 1.) Then r_enum_tree() can just put back an IS_FSL. It would affect the sort order of values prefixed with '{num}' compared to unprefixed values of the same type.
untouched (too cumbersome for summer holidays...)
.inum should be signed, or should be read with strtoul instead of strtol.
strtoul(3) used
I suppose it and cmp in the function should be ber_int_t, since LDAP integers are typically 32-bit.
Not sure what you mean.
Also, for reading .inum and syntax checking: Maybe you should use strtol() without first doing strchr(,IX_FSR), and then check that the character after the integer is IX_FSR. Slower though.
I think in this case the code is safe as is, because there's no guarantee of itmp.bv_val being nul-terminated, so better check before using strto[u]l, which expects nul-terminated strings.
Other quirks:
ldif.c uses "struct ldif_info *ni" = be->be_private. Maybe it used back-null as a template, where "ni" stands for null_info?
s/ni/li/g
Return values from get_entry() and spew_entry() are pointlessly cast to the same type as the functions already return.
Fixed.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. 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 ---------------------------------------