Re: ITS#7239: testbed - openldap-bugs

20 Apr 2012


      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