[ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

[Ben Pfaff]

On Sat, Jan 06, 2018 at 01:56:20PM +0800, Yang, Yi wrote:
> On Fri, Jan 05, 2018 at 04:42:00AM +0800, Ben Pfaff wrote:
> > On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> > > Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> > 
> > This fails to build with Clang (and, I would guess, MSVC):
> >     ../lib/odp-execute.c:497:21: error: fields must have a constant size: 'variable length array in structure' extension will never be supported
> > 
> > "sparse" issues some warnings.  This one is probably sensible:
> >     ../lib/odp-util.c:1884:32: warning: incorrect type in argument 1 (different base types)
> >     ../lib/odp-util.c:1884:32:    expected unsigned int [unsigned] [usertype] x
> >     ../lib/odp-util.c:1884:32:    got restricted ovs_be32 [assigned] [usertype] spi
> > 
> > These I don't understand.
> > https://marc.info/?l=linux-sparse&m=110218288411207 suggests that they
> > might be a sparse bug:
> > 
> >     ../lib/odp-util.c:7123:32: warning: crazy programmer
> >     ../lib/odp-util.c:7123:43: warning: crazy programmer
> >     ../lib/odp-util.c:7123:22: warning: crazy programmer
> > 
> > In odp_execute_actions(), this looks bogus: there is nothing to
> > guarantee that 'buffer' is properly aligned for struct nsh_hdr.
> > +            uint8_t buffer[NSH_HDR_MAX_LEN];
> > +            struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, buffer);
> > 
> > Similarly in format_odp_action().
> > 
> > In nsh_key_to_attr(), can this:
> > +        for (int i = 0; i < 4; i++) {
> > +            md1.context[i] = nsh->context[i];
> > +        }
> > +        nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, &md1, sizeof md1);
> > be written as just this?
> > +        nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, nsh->context,
> > +                          sizeof nsh->context);
> > and similarly in a second place.
> > 
> > In parse_odp_push_nsh_action(), the idea of xmalloc()'ing a stub is
> > weird.  A stub should be a local array.  There are many examples in the
> > tree.
> > 
> > Please don't check a pointer for null before calling free():
> > +    if (metadata != NULL) {
> > +        free(metadata);
> >      }
> 
> Thanks, Ben, I have posted v7
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/342698.html
> to fix the above issues.
> 
> By the way, what command do you run to do static code analysis by sparse?
> "make clang-analyze" will have too much warnings. I can't get sparse
> warnings, it will be better if I can run it locally.

Just follow the instructions in the documentation.