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.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
