[ovs-discuss] [PATCH] mgmt: Cleanup handling of extended messages

Ben Pfaff blp at nicira.com
Tue Aug 25 22:05:46 UTC 2009


Justin Pettit <jpettit at nicira.com> writes:

> OpenFlow has a maximum messages size of 65536 bytes, but management
> messages can be greater than that.  The management protocol's Extended
> Data message is used to get around that limitation.  This commit cleans
> up some problems with our implementation and adds some additional
> sanity-checking to received messages.

...

> @@ -54,6 +54,7 @@ static struct rconn *mgmt_rconn;
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(60, 60);
>  static struct svec capabilities;
>  static struct ofpbuf ext_data_buffer;
> +uint32_t ext_data_xid = UINT32_MAX;
>  uint64_t mgmt_id;

I believe that ext_data_xid could be static.

I think that mgmt_id could/should be static too.  I think that
was the intention, since mgmt.h declares a function
mgmt_get_mgmt_id(), but that function isn't implemented and
bridge.c actually has an explicit "extern" for mgmt_id.  It'd be
great to clean that up (which could be a separate commit).

> @@ -292,12 +297,10 @@ send_openflow_buffer(struct ofpbuf *buffer)
>                      &new_buffer);
>              oed->type = header->type;
>  
> -            if (remain > 65535) {
> +            if (remain - new_len > 0) {

This makes me think too hard, could it be written "if (remain > new_len)"?

By the way, the use of rconn_send_with_limit() in this function
makes me nervous.  This function will destroy and discard packets
if the queue gets too big.  Is that really what you want, or
would some other kind of queue limiting be a better approach?  At
any rate, it does not make sense to me to have the possibility
that some fragments of a management message get through and then
later ones are discarded.  To avoid that I would check for room
before getting started and then simply use rconn_send() with the
counter but without a limit.  e.g. just after the if
(!mgmt_rconn) { ... } do something like
        if (rconn_counter_read(txqlen) >= TXQ_LIMIT) {
            return EAGAIN;
        }
and after that just do rconn_send(..., txqlen) instead of
rconn_send_with_limit().  But all of that is predicated on
limiting this way being a good idea, and I have no idea whether
that is the case.

> @@ -670,23 +673,48 @@ static int
>  recv_ofmp_extended_data(uint32_t xid, const struct ofmp_header *ofmph,
>          size_t len)
>  {
> -    size_t data_len;
> +    int data_len;
>      struct ofmp_extended_data *ofmped;
> -    uint8_t *ptr;
>  
>      data_len = len - sizeof(*ofmped);
> -    if (data_len <= sizeof(*ofmped)) {
> +    if (data_len <= 0) {

This comparison depends on SIZE_MAX - (small number) being
negative when it is converted to int.  Maybe that is always the
case, but it makes me nervous.  I would be more comfortable with
"if (len <= sizeof(*ofmped))", which doesn't require the same
assumption.

>      if (!ofmped->flags & OFMPEDF_MORE_DATA) {

Surely this should be
      if (!(ofmped->flags & OFMPEDF_MORE_DATA)) {
right?




More information about the discuss mailing list