-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/27/2012 11:25 AM, Howard Chu wrote:
Ondrej Kuznik wrote:
- better connection handling (connections should have stable identifiers)
So far, I have had no revelation on how to proceed on this one.
What do you intend to do with such a connection identifier? If it's merely for monitor purposes, just use a simple counter. Obviously nothing in back-ldap currently needs such a thing, so I can't imagine that it would be used for anything else. You can increment the counter in getconn, when the connection is inserted into the tree (under the mutex).
It is merely for monitoring purposes, such that it is always obvious which entry corresponds to which connection over repeated searches in cn=monitor.
Your patch 4 seems to be quite invasive already. How will the new monitor output compare to the existing output? Will this break any scripts that were using the old monitor entries?
Thanks for the input, when comparing the data before submission briefly, I missed the difference that olmDbURIList and labeledURI are in "cn=Connections,cn=database #,..." in HEAD while after my patching they ended up in the "cn=database #,..." monitor entry (where they make more sense but could indeed make for an incompatible change). I will update this in the next patchset. There should be no other compatibility issues as there was no other useful information and slapo-chain seems not to actually enable monitoring of its backends (fixing that would make for another change I do not have ready).
You seem to have no-op'd out the monitor_db_close() cleanups. It would be a good idea to do all of the proper unregister/cleanup here.
Since I dropped the ad-hoc entry creation, there are now only the monitor subsystems registered, for which there currently is no unregister callback. During slapd shutdown back-monitor handles the destruction and callbacks itself, the only time the no-oping could hurt is when SLAPD_CONFIG_DELETE is enabled (as mentioned in the comment).
I can certainly add that callback to back-monitor bi->extra and use it, however such change would either be a little naive or require changing the actual type of the monitor_subsys variable to an AVL or an LDAP_LIST_*. I did not feel confident mucking with this part of back-monitor myself and judged that confining the intrusive changes only to back-ldap/monitor.c would make the patchset's chances of inclusion somewhat higher.
As you are the person to make the call, what is your opinion on this? Shall I update the patch to do proper subsystem deregistration (and prepare another that introduces such a thing to back-monitor), leave that part as it is or do something completely different?
- -- Ondrej Kuznik
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you for understanding.