On Wed, Jul 25, 2012 at 09:02:13AM +0900, Simon Horman wrote:
> On Tue, Jul 24, 2012 at 01:37:35PM -0700, Ben Pfaff wrote:
> > On Mon, Jul 23, 2012 at 03:16:36PM +0900, Simon Horman wrote:
> > > Make ofperr_encode_msg__() use correct Open Flow version in the header
> > > of messages.
> > >
> > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >
> > Looks reasonable, thanks. It doesn't apply cleanly on current master
> > otherwise I'd just push it now.
> >
> > Should we go a step further and eliminate struct ofperr_domain
> > entirely from the public interface, using the OpenFlow version
> > instead?
>
> I think that could work out well, though I'm not entirely sure what
> daemons may be lurking.
Here is the patch I cooked up.
I have changed the ofperr_domain_from_version such that it uses
NOT_REACHED() for unknown OF versions. This simplifies its callers
(all in ofp-error.c) a little. I'm unsure what ill side effects it may have.
----------------------------------------------------------------------
ofp-error: Remove ofperr_domain from external API
It seems that ofp_version suffices in all cases.
Signed-off-by: Simon Horman <ho...@verge.net.au>
diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index 9f15fac..7f65ef0 100644
--- a/lib/ofp-errors.c
+++ b/lib/ofp-errors.c
@@ -21,19 +21,26 @@ struct pair {
* 'version' (one of the possible values of struct ofp_header's 'version'
* member). Returns NULL if the version isn't defined or isn't understood by
* OVS. */
-const struct ofperr_domain *
-ofperr_domain_from_version(uint8_t version)
+static const struct ofperr_domain *
+ofperr_domain_from_version(enum ofp_version version)
{
- return (version == ofperr_of10.version ? &ofperr_of10
- : version == ofperr_of11.version ? &ofperr_of11
- : version == ofperr_of12.version ? &ofperr_of12
- : NULL);
+ switch (version) {
+ case OFP10_VERSION:
+ return &ofperr_of10;
+ case OFP11_VERSION:
+ return &ofperr_of11;
+ case OFP12_VERSION:
+ return &ofperr_of12;
+ default:
+ NOT_REACHED();
+ }
}
/* Returns the name (e.g. "OpenFlow 1.0") of OpenFlow error domain 'domain'. */
const char *
-ofperr_domain_get_name(const struct ofperr_domain *domain)
+ofperr_domain_get_name(enum ofp_version version)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return domain->name;
}
@@ -71,8 +78,9 @@ ofperr_is_nx_extension(enum ofperr error)
* A given error may not be encodable in some domains because each OpenFlow
* version tends to introduce new errors and retire some old ones. */
bool
-ofperr_is_encodable(enum ofperr error, const struct ofperr_domain *domain)
+ofperr_is_encodable(enum ofperr error, enum ofp_version version)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return (ofperr_is_valid(error)
&& domain->errors[error - OFPERR_OFS].code >= 0);
}
@@ -80,16 +88,18 @@ ofperr_is_encodable(enum ofperr error, const struct
ofperr_domain *domain)
/* Returns the OFPERR_* value that corresponds to 'type' and 'code' within
* 'domain', or 0 if no such OFPERR_* value exists. */
enum ofperr
-ofperr_decode(const struct ofperr_domain *domain, uint16_t type, uint16_t code)
+ofperr_decode(enum ofp_version version, uint16_t type, uint16_t code)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return domain->decode(type, code);
}
/* Returns the OFPERR_* value that corresponds to the category 'type' within
* 'domain', or 0 if no such OFPERR_* value exists. */
enum ofperr
-ofperr_decode_type(const struct ofperr_domain *domain, uint16_t type)
+ofperr_decode_type(enum ofp_version version, uint16_t type)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return domain->decode_type(type);
}
@@ -145,20 +155,15 @@ ofperr_get_pair__(enum ofperr error, const struct
ofperr_domain *domain)
}
static struct ofpbuf *
-ofperr_encode_msg__(enum ofperr error, uint8_t ofp_version,
+ofperr_encode_msg__(enum ofperr error, enum ofp_version version,
ovs_be32 xid, const void *data, size_t data_len)
{
struct ofp_error_msg *oem;
const struct pair *pair;
struct ofpbuf *buf;
- const struct ofperr_domain *domain;
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
- domain = ofperr_domain_from_version(ofp_version);
- if (!domain) {
- return NULL;
- }
-
- if (!ofperr_is_encodable(error, domain)) {
+ if (!ofperr_is_encodable(error, version)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
if (!ofperr_is_valid(error)) {
@@ -238,7 +243,8 @@ ofperr_encode_reply(enum ofperr error, const struct
ofp_header *oh)
* Returns NULL if 'error' is not an OpenFlow error code or if 'error' cannot
* be encoded in 'domain'. */
struct ofpbuf *
-ofperr_encode_hello(enum ofperr error, uint8_t ofp_version, const char *s)
+ofperr_encode_hello(enum ofperr error, enum ofp_version ofp_version,
+ const char *s)
{
switch (ofp_version) {
case OFP10_VERSION:
@@ -259,8 +265,9 @@ ofperr_encode_hello(enum ofperr error, uint8_t ofp_version,
const char *s)
*
* 'error' must be a valid OFPERR_* code, as checked by ofperr_is_valid(). */
int
-ofperr_get_type(enum ofperr error, const struct ofperr_domain *domain)
+ofperr_get_type(enum ofperr error, enum ofp_version version)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return ofperr_get_pair__(error, domain)->type;
}
@@ -270,8 +277,9 @@ ofperr_get_type(enum ofperr error, const struct
ofperr_domain *domain)
*
* 'error' must be a valid OFPERR_* code, as checked by ofperr_is_valid(). */
int
-ofperr_get_code(enum ofperr error, const struct ofperr_domain *domain)
+ofperr_get_code(enum ofperr error, enum ofp_version version)
{
+ const struct ofperr_domain *domain = ofperr_domain_from_version(version);
return ofperr_get_pair__(error, domain)->code;
}
@@ -285,7 +293,6 @@ ofperr_decode_msg(const struct ofp_header *oh, struct
ofpbuf *payload)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
- const struct ofperr_domain *domain;
const struct ofp_error_msg *oem;
enum ofpraw raw;
uint16_t type, code;
@@ -304,12 +311,6 @@ ofperr_decode_msg(const struct ofp_header *oh, struct
ofpbuf *payload)
}
oem = ofpbuf_pull(&b, sizeof *oem);
- /* Check version. */
- domain = ofperr_domain_from_version(oh->version);
- if (!domain) {
- return 0;
- }
-
/* Get the error type and code. */
type = ntohs(oem->type);
code = ntohs(oem->code);
@@ -330,9 +331,9 @@ ofperr_decode_msg(const struct ofp_header *oh, struct
ofpbuf *payload)
/* Translate the error type and code into an ofperr.
* If we don't know the error type and code, at least try for the type. */
- error = ofperr_decode(domain, type, code);
+ error = ofperr_decode(oh->version, type, code);
if (!error) {
- error = ofperr_decode_type(domain, type);
+ error = ofperr_decode_type(oh->version, type);
}
if (error && payload) {
ofpbuf_use_const(payload, b.data, b.size);
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index dc99d45..bb74d82 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -21,6 +21,8 @@
#include <stddef.h>
#include <stdint.h>
+#include "openflow/openflow-common.h"
+
struct ds;
struct ofp_header;
struct ofpbuf;
@@ -495,30 +497,24 @@ enum ofperr {
OFPERR_OFPET_EXPERIMENTER,
};
-extern const struct ofperr_domain ofperr_of10;
-extern const struct ofperr_domain ofperr_of11;
-extern const struct ofperr_domain ofperr_of12;
-
-const struct ofperr_domain *ofperr_domain_from_version(uint8_t version);
-const char *ofperr_domain_get_name(const struct ofperr_domain *);
+const char *ofperr_domain_get_name(enum ofp_version);
bool ofperr_is_valid(enum ofperr);
bool ofperr_is_category(enum ofperr);
bool ofperr_is_nx_extension(enum ofperr);
-bool ofperr_is_encodable(enum ofperr, const struct ofperr_domain *);
+bool ofperr_is_encodable(enum ofperr, enum ofp_version);
-enum ofperr ofperr_decode(const struct ofperr_domain *,
- uint16_t type, uint16_t code);
-enum ofperr ofperr_decode_type(const struct ofperr_domain *, uint16_t type);
+enum ofperr ofperr_decode(enum ofp_version, uint16_t type, uint16_t code);
+enum ofperr ofperr_decode_type(enum ofp_version, uint16_t type);
enum ofperr ofperr_from_name(const char *);
enum ofperr ofperr_decode_msg(const struct ofp_header *,
struct ofpbuf *payload);
struct ofpbuf *ofperr_encode_reply(enum ofperr, const struct ofp_header *);
-struct ofpbuf *ofperr_encode_hello(enum ofperr, uint8_t ofp_version,
+struct ofpbuf *ofperr_encode_hello(enum ofperr, enum ofp_version,
const char *);
-int ofperr_get_type(enum ofperr, const struct ofperr_domain *);
-int ofperr_get_code(enum ofperr, const struct ofperr_domain *);
+int ofperr_get_type(enum ofperr, enum ofp_version);
+int ofperr_get_code(enum ofperr, enum ofp_version);
const char *ofperr_get_name(enum ofperr);
const char *ofperr_get_description(enum ofperr);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 0645769..88842d8 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2627,17 +2627,10 @@ ofctl_print_error(int argc OVS_UNUSED, char *argv[])
}
for (version = 0; version <= UINT8_MAX; version++) {
- const struct ofperr_domain *domain;
-
- domain = ofperr_domain_from_version(version);
- if (!domain) {
- continue;
- }
-
printf("%s: %d,%d\n",
- ofperr_domain_get_name(domain),
- ofperr_get_type(error, domain),
- ofperr_get_code(error, domain));
+ ofperr_domain_get_name(version),
+ ofperr_get_type(error, version),
+ ofperr_get_code(error, version));
}
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
