> The "bundle" term is already used, with a different meaning, in the > ofproto-private.h interface. We should rename one of them or the > other; I'm open to either. (I'd be inclined toward renaming the > action, since "bundle" isn't really a verb, and we should at least try > to give our actions names that indicate what they do, not that we've > been consistent about that in the past.)
Hmm, I was thinking of bundle in it's verb form, like "bundle them together". Do you have any suggestions for a good name? Perhaps aggregate? I dunno, i suppose it doesn't bother me too much that the names conflict because they are in completely different contexts (an action vs a dpif concept). I'm fine with changing it though. > I know that I suggested giving NX_BD_ALG_HRW and NX_MP_ALG_HRW the > same values, but actually looking at it, I don't think it's a good > idea anymore. It's going to be hard to maintain as we add more > algorithms. Let's drop that part. Yah I was wondering about that. Especially considering that multipath is more-or-less obsolete (no users that I know of) it could possibly be going away, in which case this would be extra awkward. >> + ofpbuf_clear(b); >> + ofpbuf_prealloc_headroom(b, sizeof *nab); > > This strategy looks like a mistake. It will, I believe, cause any > action already added to 'b' to be erased, but I don't know of a reason > that "bundle" couldn't be preceded by some other action. It would be > better to just "put" space for the header before we start adding > slaves (and then keep track of its position as an offset from > b->base). Good catch, I'll add a unit test which exercises this error condition. > >> + /* Slaves array must be multiple of 8 bytes long. */ >> + ofpbuf_put_zeros(b, 8 - b->size % 8); > > That will put eight extra zero bytes if b->size is already a multiple > of 8. That will work but it's wasteful. I'd either test for b->size > % 8 == 0 in an 'if' statement or use ROUND_UP(b->size, 8) - b->size. This was just a bug. I'll fix it. Thanks for the review, I'll have an incremental out shortly. Ethan _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
