[ovs-dev] flow: Refactor flow_compose() API.
Ilya Maximets
i.maximets at samsung.com
Thu Jul 27 09:09:50 UTC 2017
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 at 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 at 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);
>
More information about the dev
mailing list