[ovs-dev] [PATCH v2 06/21] ovn-controller: Factor encapsulation code out of chassis code.

[Ben Pfaff]

On Tue, Jul 28, 2015 at 04:53:24PM -0700, Justin Pettit wrote:
> 
> 
> > On Jul 28, 2015, at 4:18 PM, Justin Pettit <jpettit at nicira.com> wrote:
> > 
> > Acked-by: Justin Pettit <jpettit at nicira.com>
> 
> I should amend my Ack.  I think this introduces a problem.
> 
> > 
> >> On Jul 28, 2015, at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:
> >> 
> >> 
> >> /* Returns true if the database is all cleaned up, false if more work is
> >> * required. */
> >> bool
> >> -chassis_cleanup(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int)
> >> +chassis_cleanup(struct controller_ctx *ctx)
> >> {
> >> -    if (!ctx->ovnsb_idl_txn || !ctx->ovs_idl_txn) {
> >> -        return false;
> >> -    }
> 
> I think you still want to check if "ctx->ovnsb_idl_txn" is null.

If you look a little farther, the code here still checks ovnsb_idl_txn,
but only if it needs to actually change anything:

    /* Delete Chassis row. */
    const struct sbrec_chassis *chassis_rec
        = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
    if (!chassis_rec) {
        return true;
    }
    if (ctx->ovnsb_idl_txn) {
        ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
                                  "ovn-controller: unregistering chassis '%s'",
                                  ctx->chassis_id);
        sbrec_chassis_delete(chassis_rec);
    }
    return false;


> >> /* Returns true if the database is all cleaned up, false if more work is
> >> * required. */
> >> bool
> >> -chassis_cleanup(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int)
> >> +encaps_cleanup(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int)
> >> {
> >> -    if (!ctx->ovnsb_idl_txn || !ctx->ovs_idl_txn) {
> >> -        return false;
> >> -    }
> 
> And, here, I think you want to check if "ctx->ovs_idl_txn" is null.

Same deal here.

> If you agree, there's no reason to repost these patches.

I think you should take another look, just to be sure.