On Thu, Jul 31, 2025 at 2:22 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 7/28/25 8:57 AM, Ales Musil wrote:
> > There have been multiple definitions of the put_load() function
> > through out the code base. There have also been variation of
> > put_load that allows to set BE array instead of LE constant.
> > Move both variations to ovn-util and djust the code as needed.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
>
> Hi Ales,
>
> Thanks for the cleanup!
>
> > controller/lflow.c | 51 +++++++------------------------------------
> > controller/physical.c | 12 ----------
> > controller/pinctrl.c | 16 --------------
> > lib/actions.c | 12 ----------
> > lib/ovn-util.c | 19 ++++++++++++++++
> > lib/ovn-util.h | 6 +++++
> > 6 files changed, 33 insertions(+), 83 deletions(-)
> >
>
> [...]
>
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index d7e236059..c8d10f493 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -1484,3 +1484,22 @@ ovn_mirror_port_name(const char *datapath_name,
> > {
> > return xasprintf("mp-%s-%s", datapath_name, port_name);
> > }
> > +
> > +void
> > +put_load_be(const void *value, size_t len, enum mf_field_id dst,
> > + size_t ofs, size_t n_bits, struct ofpbuf *ofpacts)
> > +{
> > + struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts,
> > + mf_from_id(dst),
> NULL,
> > + NULL);
> > + bitwise_copy(value, len, 0, sf->value, sf->field->n_bytes, ofs,
> n_bits);
> > + bitwise_one(ofpact_set_field_mask(sf), sf->field->n_bytes, ofs,
> n_bits);
> > +}
> > +
>
> I find the name "put_load_be()" confusing. That's because the function
> copies the value in host endianess which may or may not be BE.
>
> Would it make sense to call this function put_load_bytes()?
>
> > +void
> > +put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
> > + struct ofpbuf *ofpacts)
> > +{
> > + ovs_be64 n_value = htonll(value);
> > + put_load_be(&n_value, 8, dst, ofs, n_bits, ofpacts);
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index c05bfc53b..eddb07f4d 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -16,6 +16,7 @@
> > #ifndef OVN_UTIL_H
> > #define OVN_UTIL_H 1
> >
> > +#include "openvswitch/meta-flow.h"
> > #include "ovsdb-idl.h"
> > #include "lib/packets.h"
> > #include "lib/sset.h"
> > @@ -504,6 +505,11 @@ const struct sbrec_port_binding
> *lport_lookup_by_name(
> > struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > const char *name);
> >
> > +void put_load_be(const void *value, size_t len, enum mf_field_id dst,
> > + size_t ofs, size_t n_bits, struct ofpbuf *ofpacts);
> > +void put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
> > + struct ofpbuf *ofpacts);
> > +
>
> Nit: size_t for 'ofs' and 'n_bits'
>
> > /* __NARG__ provides a way to count the number of arguments passed to a
> > * variadic macro. As defined below, it's capable of counting up to
> > * 16 arguments. This should be more than enough for our purposes.
> However
>
> With these minor comments addressed:
>
> Acked-by: Dumitru Ceara <dce...@redhat.com>
>
> Thanks,
> Dumitru
>
>
Thank you Dumitru,
I have addressed both comments, went ahead and merged this into main.
Regards,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
