So here's my feedback on this patch. If you want to continue iterating, please do. If not, I'll address it. :)
On Mar 5, 2013, at 5:09 PM, Saul St. John wrote:
> +class nx_reg_move(of.ofp_action_vendor_base):
> + def _init(self, kw):
POX style dictates a space between class/function name and the opening paren.
> + self.vendor = NX_VENDOR_ID
> + self.subtype = NXAST_REG_MOVE
> + self.nbits = 0
> + self.src_ofs = 0
> + self.src = None
Let's duplicate the "# an nxm_entry class" comment here for src.
> + self.dst_ofs = 0
> + self.dst = None # an nxm_entry class
> +
> + def _eq (self, other):
> + if self.subtype != other.subtype: return False
> + return True
This would seem to be totally deficient, which isn't really your fault -- it
was in the reg_load code too. I *just* pushed a fix for it in reg_load; this
should be similar.
> +
> + def _pack_body (self):
> + o = self.dst()
> + o._force_mask = False
> + dst = o.pack(omittable=False, header_only=True)[:4]
Biting off the first four bytes should no longer be necessary here, since
header_only should ensure it.
> +
> + o = self.src()
> + o._force_mask = False
> + src = o.pack(omittable=False, header_only=True)[:4]
(Same as above)
> +
> + p = struct.pack('!HHHH4s4s', self.subtype, self.nbits, self.src_ofs,
> + self.dst_ofs, src, dst)
> + return p
> +
> + def _unpack_body (self, raw, offset, avail):
> + offset,(self.subtype, self.nbits, self.src_ofs, self.dst_ofs, src, dst)
> = \
> + of._unpack('!HHHH4s4s', raw, offset)
> +
> + _,dst = nxm_entry.unpack_new(dst, 0)
> + self.dst = dst.__class__
> +
> + _,src = nxm_entry.unpack_new(src, 0)
> + self.src = src.__class__
I suspect these unpacks are broken; again this is my fault. Have you tested
them? Attached is an untested patch which I think fixes them for reg load (or
at least gets closer).
> +
> + return offset
> +
> + def _body_length (self):
> + return 16
> +
> + def _show (self, prefix):
> + s = ''
> + s += prefix + ('subtype: %s\n' % (self.subtype,))
> + s += prefix + ('nbits: %s\n' % (self.nbits,))
> + s += prefix + ('src: %s\n' % (self.src,))
> + s += prefix + ('src_ofs: %s\n' % (self.src_ofs,))
> + s += prefix + ('dst: %s\n' % (self.dst,))
> + s += prefix + ('dst_ofs: %s\n' % (self.dst_ofs,))
> + return s
>
> class nx_reg_load (of.ofp_action_vendor_base):
> def _init (self, kw):
> --
> 1.7.10.4
>
-- Murphy
fix_header_unpack.diff
Description: Binary data
