Full_Name: Michael Str�der Version: git master 762ef802f6053d712459e99c65e9443cd45affdd OS: URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (79.227.156.159) Having 'logops all' in slapd.conf seems to some write ops not being written to accesslog DB. With 'logops writes' every write operation is written to accesslog. Stripped config and example data will follow.
It seems that 'logops all' only fails if an entry is renamed which has an RDN attribute which is check by slapo-unique. It seems slapo-unique's internal search is also logged to accesslog DB. Is that correct behaviour? So this combination does *not* work: ----------------------------------- overlay unique unique_attributes uid overlay accesslog logdb "cn=accesslog" logops all ----------------------------------- This combination works correctly: ----------------------------------- overlay unique unique_attributes uid overlay accesslog logdb "cn=accesslog" logops writes session -----------------------------------
See openldap-testbed-its7239.tar.bz2 attached. You have to adjust paths in the config of course. ldapadd -H ldap://localhost:2071 \ -D "uid=diradm,ou=schulung,dc=stroeder,dc=local" -w testsecret \ -f initial.ldif ldapadd -H ldap://localhost:2071 \ -D "uid=diradm,ou=schulung,dc=stroeder,dc=local" -w testsecret \ -f modrdn.ldif Now look at the accesslog DB: ldapsearch -x -H ldap://localhost:2071 -b cn=accesslog -s one "(objectClass=auditModRDN)" => no results. Now repeat that with "logops writes" instead of "logops all": rm db/*/* ldapadd... ldapmodify... $ ldapsearch -x -H ldap://localhost:2071 -b cn=accesslog -s one "(objectClass=auditModRDN)" # extended LDIF # # LDAPv3 # base <cn=accesslog> with scope oneLevel # filter: (objectClass=auditModRDN) # requesting: ALL # # 20120418194545.000001Z, accesslog dn: reqStart=20120418194545.000001Z,cn=accesslog objectClass: auditModRDN reqStart: 20120418194545.000001Z reqEnd: 20120418194545.000002Z reqType: modrdn reqSession: 1001 reqAuthzID: uid=diradm,ou=schulung,dc=stroeder,dc=local reqDN: uid=test1,ou=schulung,dc=stroeder,dc=local reqResult: 0 reqMod: entryCSN:= 20120418194545.412289Z#000000#000#000000 reqMod: modifiersName:= uid=diradm,ou=schulung,dc=stroeder,dc=local reqMod: modifyTimestamp:= 20120418194545Z reqNewRDN: uid=test2 reqDeleteOldRDN: TRUE reqEntryUUID: 86014e61-7c47-4062-931f-39526f738016 # search result search: 2 result: 0 Success # numResponses: 2 # numEntries: 1
The attachment is unreadable; can you ftp it somewhere? Thanks, p.
masarati@aero.polimi.it wrote: > > The attachment is unreadable; can you ftp it somewhere? Thanks, p. http://www.stroeder.com/temp/openldap-testbed-its7239.tar.bz2
> This is a cryptographically signed message in MIME format. > > --------------ms020801020604080509050502 > Content-Type: text/plain; charset=ISO-8859-15 > Content-Transfer-Encoding: quoted-printable > > masarati@aero.polimi.it wrote: >>=20 >> The attachment is unreadable; can you ftp it somewhere? Thanks, p. > > http://www.stroeder.com/temp/openldap-testbed-its7239.tar.bz2 Thanks. The explanation is always simple, after one finds it. The DN of accesslog entries is generated using the reqStart AVA. When logops all and internal operations take place, multiple entries are generated with the same reqStart AVA. So an internal search which is logged because of some overlay eats up the DN of the modrdn, and be_add() called by slapo-accesslog(5) fails because the entry already exists. Although logging internal entries could be of some use, I see two solutions: - modify the value of reqStart (e.g. instead of using o_tincr for the fractional part of the timestamp, we could use an incremental counter initialized with o_tincr and incremented at each logging within the same operation. The counter could be stored somewhere in the operation structure (e.g. in o_extra) - prevent logging of internal ops; again, this could be achieved by setting a dedicated o_internal_op field in the Operation structure. Since this modification could be intrusive, I'd like to gather some comments before proceeding with a fix. p.
masarati@aero.polimi.it wrote: >> This is a cryptographically signed message in MIME format. >> >> --------------ms020801020604080509050502 >> Content-Type: text/plain; charset=ISO-8859-15 >> Content-Transfer-Encoding: quoted-printable >> >> masarati@aero.polimi.it wrote: >>> =20 >>> The attachment is unreadable; can you ftp it somewhere? Thanks, p. >> >> http://www.stroeder.com/temp/openldap-testbed-its7239.tar.bz2 > > Thanks. The explanation is always simple, after one finds it. The DN of > accesslog entries is generated using the reqStart AVA. When logops all > and internal operations take place, multiple entries are generated with > the same reqStart AVA. So an internal search which is logged because of > some overlay eats up the DN of the modrdn, and be_add() called by > slapo-accesslog(5) fails because the entry already exists. > > Although logging internal entries could be of some use, I see two solutions: > > - modify the value of reqStart (e.g. instead of using o_tincr for the > fractional part of the timestamp, we could use an incremental counter > initialized with o_tincr and incremented at each logging within the same > operation. The counter could be stored somewhere in the operation > structure (e.g. in o_extra) > > - prevent logging of internal ops; again, this could be achieved by > setting a dedicated o_internal_op field in the Operation structure. > > Since this modification could be intrusive, I'd like to gather some > comments before proceeding with a fix. Hm, we've run into this similar problem before with syncprov and introduced op->o_dont_replicate. As a general rule, accesslog is meant to log user operations, not internal operations. Currently accesslog checks (op->o_dont_replicate && op->orm_no_opattrs) to skip logging of write ops. I guess we need a more general flag instead. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
>> Thanks. The explanation is always simple, after one finds it. The DN >> of >> accesslog entries is generated using the reqStart AVA. When logops all >> and internal operations take place, multiple entries are generated with >> the same reqStart AVA. So an internal search which is logged because of >> some overlay eats up the DN of the modrdn, and be_add() called by >> slapo-accesslog(5) fails because the entry already exists. >> >> Although logging internal entries could be of some use, I see two >> solutions: >> >> - modify the value of reqStart (e.g. instead of using o_tincr for the >> fractional part of the timestamp, we could use an incremental counter >> initialized with o_tincr and incremented at each logging within the same >> operation. The counter could be stored somewhere in the operation >> structure (e.g. in o_extra) >> >> - prevent logging of internal ops; again, this could be achieved by >> setting a dedicated o_internal_op field in the Operation structure. >> >> Since this modification could be intrusive, I'd like to gather some >> comments before proceeding with a fix. > > Hm, we've run into this similar problem before with syncprov and > introduced > op->o_dont_replicate. As a general rule, accesslog is meant to log user > operations, not internal operations. Currently accesslog checks > (op->o_dont_replicate && op->orm_no_opattrs) to skip logging of write ops. > I > guess we need a more general flag instead. A definitive and simple solution, although invasive, would be to: - add a o_internal flag to the Operation structure - add a macro BE_INTERNAL(op, rs, op_type) like #define BE_INTERNAL(op, rs, op_type, rcp) \ { int save_o_internal = op->o_internal; op->o_internal = 1; *rcp = (&op->o_bd->bi_info->bi_op_bind)[op_type]( op, rs ); op->o_internal = save_o_internal; } fix all calls to internal operations (brrr....) p.
On Thu, 19 Apr 2012 06:16:16 GMT, masarati@aero.polimi.it wrote: > A definitive and simple solution, although invasive, would be to: > > - add a o_internal flag to the Operation structure > > - add a macro BE_INTERNAL(op, rs, op_type) like > > #define BE_INTERNAL(op, rs, op_type, rcp) \ > { > int save_o_internal = op->o_internal; > op->o_internal = 1; > *rcp = (&op->o_bd->bi_info->bi_op_bind)[op_type]( op, rs ); > op->o_internal = save_o_internal; > } I imagine connection.c:operation_fake_init() should set op->o_internal and any callers which disagree should unset it > fix all calls to internal operations (brrr....) I do have a patch+Perl script which wraps all operation(op, rs) calls, but it doesn't know which ops are "internal". I want it for the SlapReply cleanup (ITS#6758). The only way to make a hunt for dirty SlapReplies feasible. I suggested it when I was doing the cleanup, but IIRC nobody replied - except there were related complaints that my cleanup was too invasive. http://www.openldap.org/lists/openldap-devel/201101/msg00039.html http://folk.uio.no/hbf/OpenLDAP/wrap_slap_ops.txt Anyway, here's me trying agin again:-) -- Hallvard
> On Thu, 19 Apr 2012 06:16:16 GMT, masarati@aero.polimi.it wrote: >> A definitive and simple solution, although invasive, would be to: >> >> - add a o_internal flag to the Operation structure >> >> - add a macro BE_INTERNAL(op, rs, op_type) like >> >> #define BE_INTERNAL(op, rs, op_type, rcp) \ >> { >> int save_o_internal = op->o_internal; >> op->o_internal = 1; >> *rcp = (&op->o_bd->bi_info->bi_op_bind)[op_type]( op, rs ); >> op->o_internal = save_o_internal; >> } > > I imagine connection.c:operation_fake_init() should set > op->o_internal and any callers which disagree should unset it Good point. >> fix all calls to internal operations (brrr....) > > I do have a patch+Perl script which wraps all operation(op, rs) > calls, but it doesn't know which ops are "internal". I don't think it's something that can be done automatically. Careful code review and testing would be required. > I want it for the SlapReply cleanup (ITS#6758). The only way to > make a hunt for dirty SlapReplies feasible. I suggested it when > I was doing the cleanup, but IIRC nobody replied - except there > were related complaints that my cleanup was too invasive. > > http://www.openldap.org/lists/openldap-devel/201101/msg00039.html > http://folk.uio.no/hbf/OpenLDAP/wrap_slap_ops.txt > > Anyway, here's me trying agin again:-) Sort of recall it. Well, it looks pretty invasive, indeed. The two things should be kept separate. No preference about the order in which the two changes should be applied. I'm in no hurry to work at internal ops (mostly because I have scarce time for it and I'm worried about possible unexpected showstoppers). Maybe, if all calls for internal operations are wrapped by a macro, then ITS#6758 SlapReply cleanup could be performed just from within that macro; calls that are not judged internal would need to be explicitly wrapped, though. p.
On Thu, 19 Apr 2012 14:58:32 GMT, masarati@aero.polimi.it wrote: >> I do have a patch+Perl script which wraps all operation(op, rs) >> calls, but it doesn't know which ops are "internal". > > I don't think it's something that can be done automatically. Careful > code > review and testing would be required. > >> I want it for the SlapReply cleanup (ITS#6758). The only way to >> make a hunt for dirty SlapReplies feasible. I suggested it when >> I was doing the cleanup, but IIRC nobody replied - except there >> were related complaints that my cleanup was too invasive. >> >> >> http://www.openldap.org/lists/openldap-devel/201101/msg00039.html >> http://folk.uio.no/hbf/OpenLDAP/wrap_slap_ops.txt >> >> Anyway, here's me trying agin again:-) > > Sort of recall it. Well, it looks pretty invasive, indeed. The two > things should be kept separate. No preference about the order in > which > the two changes should be applied. I'm in no hurry to work at > internal > ops (mostly because I have scarce time for it and I'm worried about > possible unexpected showstoppers). > > Maybe, if all calls for internal operations are wrapped by a macro, > then > ITS#6758 SlapReply cleanup could be performed just from within that > macro; > calls that are not judged internal would need to be explicitly > wrapped, > though. No, I meant the opposite: The macroization is done. It could mostly be scripted and makes no semantic change unless #ifdef(SlapReply debug), which is what the macros were for. Figuring out which ops to tag as internal does, as you say, require review and testing. And one extra arg to the macros. Hallvard
On 04/19/2012 11:20 PM, Hallvard Breien Furuseth wrote: > On Thu, 19 Apr 2012 14:58:32 GMT, masarati@aero.polimi.it wrote: >>> I do have a patch+Perl script which wraps all operation(op, rs) >>> calls, but it doesn't know which ops are "internal". >> >> I don't think it's something that can be done automatically. Careful code >> review and testing would be required. >> >>> I want it for the SlapReply cleanup (ITS#6758). The only way to >>> make a hunt for dirty SlapReplies feasible. I suggested it when >>> I was doing the cleanup, but IIRC nobody replied - except there >>> were related complaints that my cleanup was too invasive. >>> >>> http://www.openldap.org/lists/openldap-devel/201101/msg00039.html >>> http://folk.uio.no/hbf/OpenLDAP/wrap_slap_ops.txt >>> >>> Anyway, here's me trying agin again:-) >> >> Sort of recall it. Well, it looks pretty invasive, indeed. The two >> things should be kept separate. No preference about the order in which >> the two changes should be applied. I'm in no hurry to work at internal >> ops (mostly because I have scarce time for it and I'm worried about >> possible unexpected showstoppers). >> >> Maybe, if all calls for internal operations are wrapped by a macro, then >> ITS#6758 SlapReply cleanup could be performed just from within that >> macro; >> calls that are not judged internal would need to be explicitly wrapped, >> though. > > No, I meant the opposite: The macroization is done. It could mostly be > scripted and makes no semantic change unless #ifdef(SlapReply debug), > which is what the macros were for. Right. > Figuring out which ops to tag as internal does, as you say, require > review and testing. And one extra arg to the macros. How do you suggest to proceed? I believe using different macros, e.g. slap_bi_op_internal, slap_be_op_internal and SLAP_OP_INTERNAL is clearer than having an additional 0/1 parameter. OTOH, adding further parameters in the future would be probably easier if we extend your macros. At this point, we need consensus on addressing ITS#6758 in the first place. p. -- Pierangelo Masarati Associate Professor Dipartimento di Ingegneria Aerospaziale Politecnico di Milano
We seem to be two guys discussing work we're not volunteering to do anytime soon, but anyway... On Fri, 20 Apr 2012 07:22:32 GMT, masarati@aero.polimi.it wrote: > On 04/19/2012 11:20 PM, Hallvard Breien Furuseth wrote: >> No, I meant the opposite: The macroization is done. It could mostly >> be >> scripted and makes no semantic change unless #ifdef(SlapReply >> debug), >> which is what the macros were for. > > Right. Actually one simpler but uglier macro is enough, without touching non-internal calls: /* * Do ACT with OP->o_internal set. * Example: WITH_INTERNAL_OP( op, rc = op->o_bd->be_add( op, rs )); */ #define WITH_INTERNAL_OP(op, act) do { \ int *const wio_intern_ = &(op)->o_internal; \ int const wio_save_ = (*wio_intern_)++; \ act; \ (*wio_intern_)--; \ assert(*wio_intern_ == wio_save_); /* Keep this while testing */ \ } while (0) Not my preference, but at least it's an option. >> Figuring out which ops to tag as internal does, as you say, require >> review and testing. And one extra arg to the macros. > > How do you suggest to proceed? If I were doing it, I'd play in a separate branch while getting a feel for how it'll end up. Never mind the exact API, it can be rewritten at this stage if it's unsufficient or overkill. Then give master whatever framework is neeed to allow the rest of the job to be done some operation calls at the time, without needing a monster commit to develop and review. Technically, if not just the wrapper macro above: > I believe using different macros, e.g. > slap_bi_op_internal, slap_be_op_internal and SLAP_OP_INTERNAL is > clearer > than having an additional 0/1 parameter. OTOH, adding further > parameters in the future would be probably easier if we extend your > macros. Fair enough. 0/1 could be enum constants, but that's more verbose. And it's just 6 macros for now: My 3 old plus 3 new. /* In proto-slap.h: */ #define slap_bi_op( bi,which,op,rs) ((&(bi)->bi_op_bind)[ which ](op, rs)) #define slap_be_op( be,which,op,rs) slap_bi_op( (be)->bd_info,which,op,rs) #define SLAP_OP( which,op,rs) slap_be_op( (op)->o_bd, which,op,rs) /* Internal operations - as above but with op->o_internal set */ LDAP_SLAPD_F (int) (slap_bi_intop) LDAP_P(( BackendInfo *bi, slap_operation_t which, Operation *op, SlapReply *rs )); #define slap_be_intop(be,which,op,rs) slap_bi_intop((be)->bd_info,which,op,rs) #define SLAP_INTOP( which,op,rs) slap_be_intop(&(op)->o_bd, which,op,rs) /* In backend.c: */ int slap_bi_intop( BackendInfo *bi, slap_operation_t which, Operation *op, SlapReply *rs) { int rc; WITH_INTERNAL_OP( rc = slap_bi_op( bi, which, op, rs )); return rc; } > At this point, we need consensus on addressing ITS#6758 in the first > place. Nah. ITS#6758 needs the macros (at least if I'm to go at it), but the macros don't need ITS#6758 since they don't change what the code does. Hallvard
moved from Incoming to Software Bugs
• 7e923c72 by Ondřej Kuzník at 2022-01-18T20:11:46+00:00 ITS#7239 Do not log internal ops