On Tue, Mar 20, 2012 at 11:43:38AM +0900, Simon Horman wrote: > On Mon, Mar 19, 2012 at 02:31:36PM -0700, Ben Pfaff wrote: > > On Mon, Mar 19, 2012 at 09:54:43AM +0900, Simon Horman wrote: > > > Signed-off-by: Simon Horman <[email protected]> > > > > This looks good to me with a few comments. > > > > The meanings of the little abbreviations we use for OpenFlow versions > > are starting to get a bit more complex, so it's probably a good idea > > to include a "key" in ofp-errors.h that explains their meanings. > > > > OFPERR_OFPERR_BAD_ROLE should be OFPERR_OFPRRFC_BAD_ROLE. > > > > One leading goal of the interface here is that code buried somewhere > > in OVS should be able to return an appropriate error code, without > > worrying what version of OpenFlow we're dealing, and then when that > > error gets sent to the controller a bit of code maps it to an > > appropriate version-specific error code. But I see a couple of > > violations of that principle in this patch. > > > > The first is that OF1.2 adopts a number of Nicira extensions. When > > this happens we want the Nicira error codes to transition into the > > OF1.2 error codes. Here's an example: OFPERR_OFPRRFC_BAD_ROLE is the > > OF1.2 version of OFPERR_NXBRC_BAD_ROLE and so there should be just a > > single entry for it that looks something like: > > > > /* NX1.0(1,513), NX1.1(1,513), OF1.2(11,2). Invalid role. */ > > OFPERR_OFPERR_BAD_ROLE, > > > > The others I see are: > > > > NXBRC_NXM_BAD_VALUE -> OFPBMC_BAD_VALUE > > NXBRC_NXM_BAD_MASK -> OFPBMC_BAD_MASK > > NXBRC_NXM_BAD_PREREQ -> OFPBMC_BAD_PREREQ > > NXBRC_NXM_DUP_TYPE -> OFPBMC_DUP_FIELD > > > > Second, in a few cases OF1.2 seems to "break apart" errors defined by > > older versions. When this happens, it would be ideal if OVS > > internally could use the more-specific error codes of later versions > > and have them translated to less-specific versions for sending in > > older protocols. Here's an example: > > > > /* OF1.1(3,5). Specific experimenter instruction unsupported. */ > > OFPERR_OFPBIC_UNSUP_EXP_INST, > > > > in your patch becomes: > > > > /* OF1.1only(3,5). Specific experimenter instruction unsupported. */ > > OFPERR_OFPBIC_UNSUP_EXP_INST, > > > > /* OF1.2(3,5). Unknown experimenter id specified. */ > > OFPERR_OFPBIC_BAD_EXPERIMENTER, > > > > /* OF1.2(3,6). Unknown instruction for experimenter id. */ > > OFPERR_OFPBIC_BAD_EXP_TYPE, > > > > but it would more usefully become: > > > > /* OF1.1only(3,5), OF1.2(3,5). Unknown experimenter id specified. */ > > OFPERR_OFPBIC_BAD_EXPERIMENTER, > > > > /* OF1.1only(3,5), OF1.2(3,6). Unknown instruction for experimenter > > id. */ > > OFPERR_OFPBIC_BAD_EXP_TYPE, > > > > that is, both of the new error codes would get translated to a single > > OF1.1 error code. My guess is that the little python script disallows > > this at the moment (it makes the error codes non-bijective), but we > > should probably add support. > > I am a little unsure of how you see this translation working, > could you explain a bit further?
ofperr_encode_reply() would translate OFPERR_OFPBIC_BAD_EXPERIMENTER to 3,5 for OF1.1 and OF1.2. ofperr_encode_reply() would translate OFPERR_OFPBIC_BAD_EXP_TYPE to 3,5 for OF1.1 and to 3,6 for OF1.2. The only difficulty is translation the other direction: should ofperr_decode(of1.1, 3, 5) return OFPERR_OFPBIC_BAD_EXPERIMENTER or OFPERR_OFPBIC_BAD_EXP_TYPE? The answer may not be important. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
