| Message ID | 1501019090-106780-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 26.07.2017 00:44, andy zhou wrote: > Currently, flow_compose_size() is only supposed to be called after > flow_compose(). I find this API to be unintuitive. > > Change flow_compose() API to take the 'size' argument, and > returns 'true' if the packet can be created, 'false' otherwise. > > This change also improves error detection and reporting when > 'size' is unreasonably small. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > --- Thanks for the refactoring. I wanted to combine flow_compose_size() and flow_compose() somehow but didn't thought about the wrapper. Looks good to me in general. One comment inline. > lib/flow.c | 53 ++++++++++++++++++++++++++++++++++---------- > lib/flow.h | 3 +-- > lib/netdev-dummy.c | 7 ++++-- > ofproto/ofproto-dpif-trace.c | 2 +- > ofproto/ofproto-dpif.c | 2 +- > ovn/controller/ofctrl.c | 2 +- > tests/test-ovn.c | 2 +- > 7 files changed, 51 insertions(+), 20 deletions(-) > > diff --git a/lib/flow.c b/lib/flow.c > index 8da9f3235d0a..39aed51837a6 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -2735,17 +2735,17 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow, > } > } > > -/* Tries to increase the size of packet composed by 'flow_compose' up to > - * 'size' bytes. Fixes all the required packet headers like ip/udp lengths > - * and l3/l4 checksums. */ > -void > -flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) > +/* Increase the size of packet composed by 'flow_compose_minimal' > + * up to 'size' bytes. Fixes all the required packet headers like > + * ip/udp lengths and l3/l4 checksums. > + * > + * 'size' needs to be larger then the current packet size. */ > +static void > +packet_expand(struct dp_packet *p, const struct flow *flow, size_t size) > { > size_t extra_size; > > - if (size <= dp_packet_size(p)) { > - return; > - } > + ovs_assert(size > dp_packet_size(p)); > > extra_size = size - dp_packet_size(p); > dp_packet_put_zeros(p, extra_size); > @@ -2754,7 +2754,6 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) > struct eth_header *eth = dp_packet_eth(p); > > eth->eth_type = htons(dp_packet_size(p)); > - > } else if (dl_type_is_ip_any(flow->dl_type)) { > uint32_t pseudo_hdr_csum; > size_t l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p); > @@ -2789,9 +2788,12 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) > * 'flow'. > * > * (This is useful only for testing, obviously, and the packet isn't really > - * valid. Lots of fields are just zeroed.) */ > -void > -flow_compose(struct dp_packet *p, const struct flow *flow) > + * valid. Lots of fields are just zeroed.) > + * > + * The created packet will have minimal packet length, just large > + * enough to hold the packet header fields. */ > +static void > +flow_compose_minimal(struct dp_packet *p, const struct flow *flow) > { > uint32_t pseudo_hdr_csum; > size_t l4_len; > @@ -2896,6 +2898,33 @@ flow_compose(struct dp_packet *p, const struct flow *flow) > } > } > } > + > +/* Puts into 'p' a Ethernet frame of size 'size' that flow_extract() would > + * parse as having the given 'flow'. > + * > + * When 'size' is zero, 'p' is a minimal size packet that only big enough > + * to contains all packet headers. > + * > + * When 'size' is larger than the minimal packet size, the packet will > + * be expended to 'size' with the payload set to zero. > + * > + * Return 'true' if the packet is successfully created. 'false' otherwise. > + * Note, when 'size' is set to zero, this function always returns true. */ > +bool > +flow_compose(struct dp_packet *p, const struct flow *flow, size_t size) > +{ > + flow_compose_minimal(p, flow); > + > + if (size && size < dp_packet_size(p)) { > + return false; > + } > + > + if (size > dp_packet_size(p)) { > + packet_expand(p, flow, size); > + } > + > + return true; > +} > > /* Compressed flow. */ > > diff --git a/lib/flow.h b/lib/flow.h > index 1bbbe410c6d2..0c6069c66c74 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -124,8 +124,7 @@ void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc); > void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack); > void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse); > > -void flow_compose(struct dp_packet *, const struct flow *); > -void flow_compose_size(struct dp_packet *, const struct flow *, size_t size); > +bool flow_compose(struct dp_packet *, const struct flow *, size_t); > > bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, > uint8_t *nw_frag); > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 83e30b37cbc3..6e110b1c7376 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -1478,8 +1478,11 @@ eth_from_flow(const char *s, size_t packet_size) > } > > packet = dp_packet_new(0); > - flow_compose(packet, &flow); > - flow_compose_size(packet, &flow, packet_size); > + if (!flow_compose(packet, &flow, packet_size)) { > + dp_packet_uninit(packet); > + free(packet); I think, it's better to use 'dp_packet_delete(packet)' instead of two lines above. 'delete' is the better pair for 'new'. 'uninit' is mostly for packets allocated statically and initialized by 'dp_packet_init()'. Additionally this will help to avoid issues if 'dp_packet_new' will be able to allocate DPDK memory someday. With this change: Acked-by: Ilya Maximets <i.maximets@samsung.com> > + packet = NULL; > + }; > > ofpbuf_uninit(&odp_key); > return packet; > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index b3f3cbc6a4ef..38d11002f290 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -376,7 +376,7 @@ parse_flow_and_packet(int argc, const char *argv[], > /* Generate a packet, if requested. */ > if (packet) { > if (!dp_packet_size(packet)) { > - flow_compose(packet, flow); > + flow_compose(packet, flow, 0); > } else { > /* Use the metadata from the flow and the packet argument > * to reconstruct the flow. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 50f440fd8964..823e506f1f7c 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1287,7 +1287,7 @@ check_ct_eventmask(struct dpif_backer *backer) > > /* Compose a dummy UDP packet. */ > dp_packet_init(&packet, 0); > - flow_compose(&packet, &flow); > + flow_compose(&packet, &flow, 0); > > /* Execute the actions. On older datapaths this fails with EINVAL, on > * newer datapaths it succeeds. */ > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > index 5aff2302a346..7164ff061b64 100644 > --- a/ovn/controller/ofctrl.c > +++ b/ovn/controller/ofctrl.c > @@ -1150,7 +1150,7 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s, > uint64_t packet_stub[128 / 8]; > struct dp_packet packet; > dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); > - flow_compose(&packet, &uflow); > + flow_compose(&packet, &uflow, 0); > > uint64_t ofpacts_stub[1024 / 8]; > struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index ca27a0f5a1a2..a216b820a02c 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -1164,7 +1164,7 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED) > uint64_t packet_stub[128 / 8]; > struct dp_packet packet; > dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); > - flow_compose(&packet, &uflow); > + flow_compose(&packet, &uflow, 0); > > struct ds output = DS_EMPTY_INITIALIZER; > const uint8_t *buf = dp_packet_data(&packet); >
> > I think, it's better to use 'dp_packet_delete(packet)' instead of two > lines above. 'delete' is the better pair for 'new'. 'uninit' is mostly > for packets allocated statically and initialized by 'dp_packet_init()'. > > Additionally this will help to avoid issues if 'dp_packet_new' will > be able to allocate DPDK memory someday. You are right. I folded the change in. Thanks for the suggestion. > > With this change: > Acked-by: Ilya Maximets <i.maximets@samsung.com> > Thanks for the review. Pushed to master.
diff --git a/lib/flow.c b/lib/flow.c index 8da9f3235d0a..39aed51837a6 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -2735,17 +2735,17 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow, } } -/* Tries to increase the size of packet composed by 'flow_compose' up to - * 'size' bytes. Fixes all the required packet headers like ip/udp lengths - * and l3/l4 checksums. */ -void -flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) +/* Increase the size of packet composed by 'flow_compose_minimal' + * up to 'size' bytes. Fixes all the required packet headers like + * ip/udp lengths and l3/l4 checksums. + * + * 'size' needs to be larger then the current packet size. */ +static void +packet_expand(struct dp_packet *p, const struct flow *flow, size_t size) { size_t extra_size; - if (size <= dp_packet_size(p)) { - return; - } + ovs_assert(size > dp_packet_size(p)); extra_size = size - dp_packet_size(p); dp_packet_put_zeros(p, extra_size); @@ -2754,7 +2754,6 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) struct eth_header *eth = dp_packet_eth(p); eth->eth_type = htons(dp_packet_size(p)); - } else if (dl_type_is_ip_any(flow->dl_type)) { uint32_t pseudo_hdr_csum; size_t l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p); @@ -2789,9 +2788,12 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) * 'flow'. * * (This is useful only for testing, obviously, and the packet isn't really - * valid. Lots of fields are just zeroed.) */ -void -flow_compose(struct dp_packet *p, const struct flow *flow) + * valid. Lots of fields are just zeroed.) + * + * The created packet will have minimal packet length, just large + * enough to hold the packet header fields. */ +static void +flow_compose_minimal(struct dp_packet *p, const struct flow *flow) { uint32_t pseudo_hdr_csum; size_t l4_len; @@ -2896,6 +2898,33 @@ flow_compose(struct dp_packet *p, const struct flow *flow) } } } + +/* Puts into 'p' a Ethernet frame of size 'size' that flow_extract() would + * parse as having the given 'flow'. + * + * When 'size' is zero, 'p' is a minimal size packet that only big enough + * to contains all packet headers. + * + * When 'size' is larger than the minimal packet size, the packet will + * be expended to 'size' with the payload set to zero. + * + * Return 'true' if the packet is successfully created. 'false' otherwise. + * Note, when 'size' is set to zero, this function always returns true. */ +bool +flow_compose(struct dp_packet *p, const struct flow *flow, size_t size) +{ + flow_compose_minimal(p, flow); + + if (size && size < dp_packet_size(p)) { + return false; + } + + if (size > dp_packet_size(p)) { + packet_expand(p, flow, size); + } + + return true; +} /* Compressed flow. */ diff --git a/lib/flow.h b/lib/flow.h index 1bbbe410c6d2..0c6069c66c74 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -124,8 +124,7 @@ void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc); void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack); void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse); -void flow_compose(struct dp_packet *, const struct flow *); -void flow_compose_size(struct dp_packet *, const struct flow *, size_t size); +bool flow_compose(struct dp_packet *, const struct flow *, size_t); bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, uint8_t *nw_frag); diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 83e30b37cbc3..6e110b1c7376 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1478,8 +1478,11 @@ eth_from_flow(const char *s, size_t packet_size) } packet = dp_packet_new(0); - flow_compose(packet, &flow); - flow_compose_size(packet, &flow, packet_size); + if (!flow_compose(packet, &flow, packet_size)) { + dp_packet_uninit(packet); + free(packet); + packet = NULL; + }; ofpbuf_uninit(&odp_key); return packet; diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index b3f3cbc6a4ef..38d11002f290 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -376,7 +376,7 @@ parse_flow_and_packet(int argc, const char *argv[], /* Generate a packet, if requested. */ if (packet) { if (!dp_packet_size(packet)) { - flow_compose(packet, flow); + flow_compose(packet, flow, 0); } else { /* Use the metadata from the flow and the packet argument * to reconstruct the flow. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 50f440fd8964..823e506f1f7c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1287,7 +1287,7 @@ check_ct_eventmask(struct dpif_backer *backer) /* Compose a dummy UDP packet. */ dp_packet_init(&packet, 0); - flow_compose(&packet, &flow); + flow_compose(&packet, &flow, 0); /* Execute the actions. On older datapaths this fails with EINVAL, on * newer datapaths it succeeds. */ diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c index 5aff2302a346..7164ff061b64 100644 --- a/ovn/controller/ofctrl.c +++ b/ovn/controller/ofctrl.c @@ -1150,7 +1150,7 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s, uint64_t packet_stub[128 / 8]; struct dp_packet packet; dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); - flow_compose(&packet, &uflow); + flow_compose(&packet, &uflow, 0); uint64_t ofpacts_stub[1024 / 8]; struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); diff --git a/tests/test-ovn.c b/tests/test-ovn.c index ca27a0f5a1a2..a216b820a02c 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -1164,7 +1164,7 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED) uint64_t packet_stub[128 / 8]; struct dp_packet packet; dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); - flow_compose(&packet, &uflow); + flow_compose(&packet, &uflow, 0); struct ds output = DS_EMPTY_INITIALIZER; const uint8_t *buf = dp_packet_data(&packet);
Currently, flow_compose_size() is only supposed to be called after flow_compose(). I find this API to be unintuitive. Change flow_compose() API to take the 'size' argument, and returns 'true' if the packet can be created, 'false' otherwise. This change also improves error detection and reporting when 'size' is unreasonably small. Signed-off-by: Andy Zhou <azhou@ovn.org> --- lib/flow.c | 53 ++++++++++++++++++++++++++++++++++---------- lib/flow.h | 3 +-- lib/netdev-dummy.c | 7 ++++-- ofproto/ofproto-dpif-trace.c | 2 +- ofproto/ofproto-dpif.c | 2 +- ovn/controller/ofctrl.c | 2 +- tests/test-ovn.c | 2 +- 7 files changed, 51 insertions(+), 20 deletions(-)