[ovs-dev] [PATCH v4 01/14] ofpbuf: New function ofpbuf_const_initializer().

[Jarno Rajahalme]

With a note and a question below:

Acked-by: Jarno Rajahalme <jarno at ovn.org>

> On Feb 19, 2016, at 12:34 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> A number of times I've looked at code and thought that it would be easier
> to understand if I could write an initializer instead of
> ofpbuf_use_const().  This commit adds a function for that purpose and
> adapts a lot of code to use it, in the places where I thought it made
> the code better.
> 
> In theory this could improve code generation since the new function can
> be inlined whereas ofpbuf_use_const() isn't.  But I guess that's probably
> insignificant; the intent of this change is code readability.
> 
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> —
> 

(snip)

> diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
> index dd72cde..489a543 100644
> --- a/lib/ofpbuf.h
> +++ b/lib/ofpbuf.h
> @@ -64,8 +64,9 @@ struct ofpbuf {
>     enum ofpbuf_source source;  /* Source of memory allocated as 'base'. */
> };
> 
> -/* An initializer for a struct ofpbuf that will be initially empty and
> - * uses the space in STUB (which should be an array) as a stub.
> +/* An initializer for a struct ofpbuf that will be initially empty and uses the
> + * space in STUB (which should be an array) as a stub.  This is the initializer
> + * form of ofpbuf_use_stub().
>  *
>  * Usage example:
>  *
> @@ -83,6 +84,27 @@ struct ofpbuf {
>         .source = OFPBUF_STUB,                  \
>     }
> 
> +/* An initializer for a struct ofpbuf whose data starts at DATA and continues
> + * for SIZE bytes.  This is appropriate for an ofpbuf that will be used to
> + * inspect existing data, without moving it around or reallocating it, and
> + * generally without modifying it at all.  This is the initializer form of
> + * ofpbuf_use_const().
> + */
> +static inline struct ofpbuf
> +ofpbuf_const_initializer(const void *data, size_t size)
> +{
> +    return (struct ofpbuf) {
> +        .base = CONST_CAST(void *, data),
> +        .data = CONST_CAST(void *, data),
> +        .size = size,
> +        .allocated = size,
> +        .header = NULL,
> +        .msg = NULL,
> +        .list_node = OVS_LIST_POISON,
> +        .source = OFPBUF_STACK,
> +    };
> +}
> +

I’m curious if there is a specific reason to define ofpbuf_const_initializer() as an inline function, and keep the OFPBUF_STUB_INITIALIZER() as a macro?

> void ofpbuf_use_ds(struct ofpbuf *, const struct ds *);
> void ofpbuf_use_stack(struct ofpbuf *, void *, size_t);
> void ofpbuf_use_stub(struct ofpbuf *, void *, size_t);

(snip)

> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 97b7f6c..7b7532d 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -70,6 +70,15 @@ set_switch_config(struct rconn *swconn,
>     queue_msg(request);
> }
> 
> +unsigned int init_ofpbuf(const void *p, size_t size);
> +unsigned int
> +init_ofpbuf(const void *p, size_t size)
> +{
> +    struct ofpbuf b = ofpbuf_const_initializer(p, size);
> +    ofpbuf_pull(&b, 8);
> +    return b.size;
> +}
> +

This seems unused.

> static void
> process_packet_in(struct controller_ctx *ctx OVS_UNUSED,
>                   const struct ofp_header *msg)

(snip)