Re: (ITS#4627) back-ldif issues

[ando@sys-net.it]

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
---------------------------------------