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 <ho...@verge.net.au> > > 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? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
