Oops, the last comment was meant for patch 9 of the series. I will
repost it there as well.
On Mon, Mar 21, 2011 at 5:40 PM, Ethan Jackson <et...@nicira.com> wrote:
> I think you missed one in handle_role_request line 4325. Otherwise looks
> good.
>
> On Tue, Mar 15, 2011 at 11:36 AM, Ben Pfaff <b...@nicira.com> wrote:
>> This helps to increase the level of abstraction of "struct ofconn",
>> in preparation for moving it from ofproto.c into a new file.
>> ---
>> ofproto/ofproto.c | 54
>> ++++++++++++++++++++++++++++++----------------------
>> 1 files changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 23069eb..bbceae2 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -2520,20 +2520,26 @@ queue_tx(struct ofpbuf *msg, const struct ofconn
>> *ofconn,
>> }
>>
>> static void
>> +ofconn_send_reply(const struct ofconn *ofconn, struct ofpbuf *msg)
>> +{
>> + queue_tx(msg, ofconn, ofconn->reply_counter);
>> +}
>> +
>> +static void
>> send_error_oh(const struct ofconn *ofconn, const struct ofp_header *oh,
>> int error)
>> {
>> struct ofpbuf *buf = ofputil_encode_error_msg(error, oh);
>> if (buf) {
>> COVERAGE_INC(ofproto_error);
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>> }
>> }
>>
>> static int
>> handle_echo_request(struct ofconn *ofconn, const struct ofp_header *oh)
>> {
>> - queue_tx(make_echo_reply(oh), ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, make_echo_reply(oh));
>> return 0;
>> }
>>
>> @@ -2568,7 +2574,7 @@ handle_features_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> hton_ofp_phy_port(ofpbuf_put(buf, &port->opp, sizeof port->opp));
>> }
>>
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>> return 0;
>> }
>>
>> @@ -2589,7 +2595,7 @@ handle_get_config_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> osc = make_openflow_xid(sizeof *osc, OFPT_GET_CONFIG_REPLY, oh->xid,
>> &buf);
>> osc->flags = htons(flags);
>> osc->miss_send_len = htons(ofconn->miss_send_len);
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>>
>> return 0;
>> }
>> @@ -3275,7 +3281,7 @@ append_ofp_stats_reply(size_t nbytes, struct ofconn
>> *ofconn,
>> struct ofp_stats_reply *reply = msg->data;
>> reply->flags = htons(OFPSF_REPLY_MORE);
>> *msgp = make_ofp_stats_reply(reply->header.xid, reply->type, nbytes);
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>> }
>> return ofpbuf_put_uninit(*msgp, nbytes);
>> }
>> @@ -3311,7 +3317,7 @@ append_nxstats_reply(size_t nbytes, struct ofconn
>> *ofconn,
>> struct nicira_stats_msg *reply = msg->data;
>> reply->flags = htons(OFPSF_REPLY_MORE);
>> *msgp = make_nxstats_reply(reply->header.xid, reply->subtype,
>> nbytes);
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>> }
>> ofpbuf_prealloc_tailroom(*msgp, nbytes);
>> }
>> @@ -3332,7 +3338,7 @@ handle_desc_stats_request(struct ofconn *ofconn,
>> ovs_strlcpy(ods->sw_desc, p->sw_desc, sizeof ods->sw_desc);
>> ovs_strlcpy(ods->serial_num, p->serial_desc, sizeof ods->serial_num);
>> ovs_strlcpy(ods->dp_desc, p->dp_desc, sizeof ods->dp_desc);
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>>
>> return 0;
>> }
>> @@ -3358,7 +3364,7 @@ handle_table_stats_request(struct ofconn *ofconn,
>> put_32aligned_be64(&ots->lookup_count, htonll(0)); /* XXX */
>> put_32aligned_be64(&ots->matched_count, htonll(0)); /* XXX */
>>
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>> return 0;
>> }
>>
>> @@ -3412,7 +3418,7 @@ handle_port_stats_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> }
>> }
>>
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>> return 0;
>> }
>>
>> @@ -3507,7 +3513,7 @@ handle_flow_stats_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> put_ofp_flow_stats(ofconn, rule, fsr->out_port, &reply);
>> }
>> }
>> - queue_tx(reply, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, reply);
>>
>> return 0;
>> }
>> @@ -3584,7 +3590,7 @@ handle_nxst_flow(struct ofconn *ofconn, const struct
>> ofp_header *oh)
>> put_nx_flow_stats(ofconn, rule, nfsr->out_port, &reply);
>> }
>> }
>> - queue_tx(reply, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, reply);
>>
>> return 0;
>> }
>> @@ -3680,7 +3686,7 @@ handle_aggregate_stats_request(struct ofconn *ofconn,
>> reply = append_ofp_stats_reply(sizeof *reply, ofconn, &msg);
>> query_aggregate_stats(ofproto, &target, request->out_port,
>> request->table_id, reply);
>> - queue_tx(msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, msg);
>> return 0;
>> }
>>
>> @@ -3713,7 +3719,7 @@ handle_nxst_aggregate(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> reply = ofpbuf_put_uninit(buf, sizeof *reply);
>> query_aggregate_stats(ofproto, &target, request->out_port,
>> request->table_id, reply);
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>>
>> return 0;
>> }
>> @@ -3801,7 +3807,7 @@ handle_queue_stats_request(struct ofconn *ofconn,
>> const struct ofp_header *oh)
>> ofpbuf_delete(cbdata.msg);
>> return ofp_mkerr(OFPET_QUEUE_OP_FAILED, OFPQOFC_BAD_PORT);
>> }
>> - queue_tx(cbdata.msg, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, cbdata.msg);
>>
>> return 0;
>> }
>> @@ -4209,7 +4215,7 @@ handle_role_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>>
>> reply = make_nxmsg_xid(sizeof *reply, NXT_ROLE_REPLY, oh->xid, &buf);
>> reply->role = htonl(role);
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>>
>> return 0;
>> }
>> @@ -4241,7 +4247,7 @@ handle_barrier_request(struct ofconn *ofconn, const
>> struct ofp_header *oh)
>> /* Currently, everything executes synchronously, so we can just
>> * immediately send the barrier reply. */
>> ob = make_openflow_xid(sizeof *ob, OFPT_BARRIER_REPLY, oh->xid, &buf);
>> - queue_tx(buf, ofconn, ofconn->reply_counter);
>> + ofconn_send_reply(ofconn, buf);
>> return 0;
>> }
>>
>> @@ -4773,18 +4779,20 @@ rule_send_removed(struct ofproto *p, struct rule
>> *rule, uint8_t reason)
>> fr.byte_count = rule->byte_count;
>>
>> LIST_FOR_EACH (ofconn, node, &p->all_conns) {
>> + struct ofpbuf *msg;
>> +
>> if (!rconn_is_connected(ofconn->rconn)
>> || !ofconn_receives_async_msgs(ofconn)) {
>> continue;
>> }
>>
>> - /* Account flow expirations under ofconn->reply_counter, the counter
>> - * for replies to OpenFlow requests. That works because preventing
>> - * OpenFlow requests from being processed also prevents new flows
>> from
>> - * being added (and expiring). (It also prevents processing
>> OpenFlow
>> - * requests that would not add new flows, so it is imperfect.) */
>> - queue_tx(ofputil_encode_flow_removed(&fr, ofconn->flow_format),
>> - ofconn, ofconn->reply_counter);
>> + /* This accounts flow expirations as if they were replies to
>> OpenFlow
>> + * requests. That works because preventing OpenFlow requests from
>> + * being processed also prevents new flows from being added (and
>> + * expiring). (It also prevents processing OpenFlow requests that
>> + * would not add new flows, so it is imperfect.) */
>> + msg = ofputil_encode_flow_removed(&fr, ofconn->flow_format);
>> + ofconn_send_reply(ofconn, msg);
>> }
>> }
>>
>> --
>> 1.7.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
