When is it safe to leave sc_writewait uninitialized? The field seems to have emerged in the middle of RE24, and I don't see any flags which say it should/should not be used.
Since not setting it crashed back-relay in ITS#8428, does that mean everything which creates a slap_callback should set it? Seems unfortunate for third-party modules...
Also, I think this should be documented.
Hallvard Breien Furuseth wrote:
When is it safe to leave sc_writewait uninitialized? The field seems to have emerged in the middle of RE24, and I don't see any flags which say it should/should not be used.
Where would such flags reside?
Since not setting it crashed back-relay in ITS#8428, does that mean everything which creates a slap_callback should set it?
Yes.
Seems unfortunate for third-party modules...
Also, I think this should be documented.
Howard Chu wrote:
Hallvard Breien Furuseth wrote:
When is it safe to leave sc_writewait uninitialized? The field seems to have emerged in the middle of RE24, and I don't see any flags which say it should/should not be used.
Where would such flags reside?
slap_callback.sc_flags. If it existed it could get a "the sc_writewait field is valid" bit. Then the struct could have been safely extended without breaking other modules - provided they were recompiled, anyway.
Since not setting it crashed back-relay in ITS#8428, does that mean everything which creates a slap_callback should set it?
Yes.
Then most callback code in slapd must be fixed. Now I don't understand why slapd still works as well as it does...
Instead of sc_flags, maybe we should add a word 'sc_signature' for now: back-mdb must set it to a particular value which indicates that sc_writewait is valid, and it must be cleared before freeing the callback. That way modules which don't know about sc_writewait _usually_ won't break, though the signature can be right once in a while.
Hallvard Breien Furuseth wrote:
Howard Chu wrote:
Hallvard Breien Furuseth wrote:
When is it safe to leave sc_writewait uninitialized? The field seems to have emerged in the middle of RE24, and I don't see any flags which say it should/should not be used.
Where would such flags reside?
slap_callback.sc_flags. If it existed it could get a "the sc_writewait field is valid" bit. Then the struct could have been safely extended without breaking other modules - provided they were recompiled, anyway.
Since not setting it crashed back-relay in ITS#8428, does that mean everything which creates a slap_callback should set it?
Yes.
Then most callback code in slapd must be fixed. Now I don't understand why slapd still works as well as it does...
Because most callback users do a default initialization, e.g. slap_callback cb = { xx };
back-relay was odd in this respect.
Howard Chu wrote:
Hallvard Breien Furuseth wrote:
Then most callback code in slapd must be fixed. Now I don't understand why slapd still works as well as it does...
Because most callback users do a default initialization, e.g. slap_callback cb = { xx };
Oh, right... New problem, though: If a a third-party module sets sc_private in such an initializer, that now sets sc_writewait instead. I don't see that in OpenLDAP code - it is C90-compatible and thus requires constant expressions in struct initializers.
Anyway, branch "writewait" in my UiO repo patches more missed cases.
Hallvard Breien Furuseth wrote:
I wrote:
Oh, right... New problem, though: If a a third-party module sets sc_private in such an initializer, that now sets sc_writewait instead.
I mean, and old module which predates sc_writewait.
Hm yeah, I suppose we could just move sc_writewait to the end of the struct. Probably should have done that in the first place.
I lost this thread (from May 19). Summarized in ITS#8435 plus a new suggestion: Revert sc_writewait and get it done with a field somewhere else, maintained by callbacks. Dunno if that's feasible.
There a lot of bugs around slap_callback.sc_writewait https://github.com/ReOpen/ReOpenLDAP/commit/80c9667d823475710bb6c94a7735c290...
Regards, Leonid.
2016-06-06 9:18 GMT+03:00 Hallvard Breien Furuseth h.b.furuseth@usit.uio.no:
I lost this thread (from May 19). Summarized in ITS#8435 plus a new suggestion: Revert sc_writewait and get it done with a field somewhere else, maintained by callbacks. Dunno if that's feasible.
On 06/06/16 08:26, Леонид Юрьев wrote:
There a lot of bugs around slap_callback.sc_writewait https://github.com/ReOpen/ReOpenLDAP/commit/80c9667d823475710bb6c94a7735c290...
Last I heard, we should stay away from your repo duo to an IPR conflict. Can you summarize, if it's more than what I got? slapd: backover.c, syncrepl.c. slapd/slapi: slapi_overlay.c. slapd/overlays: constraint.c, dds.c, dynlist.c, memberof.c, pcache.c. contrib/slapd-modules: authzid/authzid.c.
That's from git://git.uio.no/u/hbf/openldap.git branch "writewait".
Hallvard Breien Furuseth wrote:
I lost this thread (from May 19). Summarized in ITS#8435 plus a new suggestion: Revert sc_writewait and get it done with a field somewhere else, maintained by callbacks. Dunno if that's feasible.
There is no "somewhere else" to use.
I see no reason to revert the change. We can simply add sc_writewait to the end of the structure, which is the normal practice for adding new fields to existing structs.
On 08. juni 2016 23:55, Howard Chu wrote:
Hallvard Breien Furuseth wrote:
I lost this thread (from May 19). Summarized in ITS#8435 plus a new suggestion: Revert sc_writewait and get it done with a field somewhere else, maintained by callbacks. Dunno if that's feasible.
There is no "somewhere else" to use.
I see no reason to revert the change.
The reason would be trying to not break third-party modules, like I've said. But the problem is 1.5 years old and hopefully such a problem would have been reported by now, so maybe there are very few affected modules out there.
We can simply add sc_writewait to the end of the structure, which is the normal practice for adding new fields to existing structs.
Hallvard Breien Furuseth wrote:
On 08. juni 2016 23:55, Howard Chu wrote:
Hallvard Breien Furuseth wrote:
I lost this thread (from May 19). Summarized in ITS#8435 plus a new suggestion: Revert sc_writewait and get it done with a field somewhere else, maintained by callbacks. Dunno if that's feasible.
There is no "somewhere else" to use.
I see no reason to revert the change.
The reason would be trying to not break third-party modules, like I've said. But the problem is 1.5 years old and hopefully such a problem would have been reported by now, so maybe there are very few affected modules out there.
Exactly.
We can simply add sc_writewait to the end of the structure, which is the normal practice for adding new fields to existing structs.
-
Hallvard Breien Furuseth -
Howard Chu -
Леонид Юрьев