casaroli commented on code in PR #21515: URL: https://github.com/apache/nuttx/pull/21515#discussion_r4501777191
########## net/netforward/can_forward.c: ########## @@ -0,0 +1,230 @@ +/**************************************************************************** + * net/netforward/can_forward.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <string.h> +#include <assert.h> +#include <debug.h> +#include <errno.h> + +#include <nuttx/mm/iob.h> +#include <nuttx/can.h> +#include <nuttx/net/can.h> +#include <nuttx/net/net.h> +#include <nuttx/net/netdev.h> +#include <nuttx/net/netstats.h> + +#include "devif/devif.h" +#include "netforward/netforward.h" + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: can_dev_forward + * + * Description: + * This function is called from can_forward when it is necessary to + * forward a packet from the current device to different device. In this + * case, the forwarding operation must be performed asynchronously when + * the TX poll is received from the forwarding device. + * + * Input Parameters: + * dev - The device on which the packet was received and which + * contains the packet. + * fwdrule - The forward target rule. + * + * Returned Value: + * Zero is returned if the packet was successfully forward; A negated + * errno value is returned if the packet is not forwardable. + * + ****************************************************************************/ + +static int can_dev_forward(FAR struct net_driver_s *dev, + FAR struct cangw_rule_s *fwdrule) +{ + FAR struct forward_s *fwd; + FAR struct net_driver_s *fwddev = fwdrule->dest_dev; + int ret = -ENOMEM; + + /* If the interface isn't "Running", we can't forward. */ + + if ((fwddev->d_flags & IFF_RUNNING) == 0) + { + nwarn("WARNING: device is DOWN\n"); + ret = -EHOSTUNREACH; + goto errout; + } + + /* Get a pre-allocated forwarding structure, This structure will be + * completely zeroed when we receive it. + */ + + fwd = netfwd_alloc(); + if (fwd == NULL) + { + nwarn("WARNING: Failed to allocate forwarding structure\n"); + goto errout; + } + + /* Initialize the easy stuff in the forwarding structure */ + + fwd->f_dev = fwddev; /* Forwarding device */ + fwd->f_domain = PF_CAN; /* CAN domain */ + + /* Relay the device buffer */ + + fwd->f_iob = can_iob_clone(dev); /* IOB chain to be forwarded */ + if (fwd->f_iob == NULL) + { + nwarn("WARNING: Failed to clone IOB\n"); + goto errout_with_fwd; + } + + /* Then set up to forward the packet according to the protocol. */ + + ret = netfwd_forward(dev, fwd); + if (ret >= 0) + { +#ifdef CONFIG_NET_STATISTICS + g_netstats.cangw.forward++; +#endif + return OK; + } + +errout_with_fwd: + if (fwd->f_iob != NULL) + { + iob_free_chain(fwd->f_iob); + } + + netfwd_free(fwd); + +errout: +#ifdef CONFIG_NET_STATISTICS + g_netstats.cangw.drop++; +#endif + return ret; +} + +/**************************************************************************** + * Name: can_forward_filter + * + * Description: + * Check if the input id are match to one forward target rule. + * + * Input Parameters: + * filter - The forward filter. + * id - CAN msg ID. + * + * Returned Value: + * 0 - not match, 1 = match + * + ****************************************************************************/ + +static inline_function int can_forward_filter(FAR struct can_filter *filter, + canid_t id) +{ + if (id & CAN_INV_FILTER) Review Comment: `CAN_INV_FILTER` is a flag of the filter (`include/nuttx/can.h:93`: "To be set in can_filter.can_id"), not of the received frame, so this tests the wrong value. Fix: test `filter->can_id & CAN_INV_FILTER`, as `net/can/can_conn.c:202` does. ########## net/netforward/netfwd_dropstats.c: ########## @@ -55,34 +53,36 @@ * ****************************************************************************/ +#ifdef CONFIG_NET_IPFORWARD Review Comment: `proto_dropstats()` now exists only with `CONFIG_NET_IPFORWARD`, but `ipv4_dropstats()` and `ipv6_dropstats()` still call it, and this file is built for every `NET_FORWARD` with `NET_STATISTICS`. `qemu-armv8a:netnsh` with `NET_CAN` and `NET_CANFORWARD` (no `NET_IPFORWARD`) fails: `netfwd_dropstats.c:141:9: error: implicit declaration of function 'proto_dropstats'`. Fix: put `ipv4_dropstats()` and `ipv6_dropstats()` under `CONFIG_NET_IPFORWARD` too, with the IPv4/IPv6 cases of `netfwd_dropstats()`. ########## include/nuttx/net/netdev.h: ########## @@ -576,6 +576,16 @@ struct net_driver_s unsigned long arg); #endif +#ifdef CONFIG_NET_FORWARD + struct work_s d_fwdwork; /* For concurrent forwarding work */ +#endif + + /* CAN gateway */ + +#ifdef CONFIG_NET_CANFORWARD + sq_queue_t d_cangw_rules; /* CAN gateway rules */ Review Comment: No code adds a rule to `d_cangw_rules`: `cangw.h` has no function and nothing else touches the list, so `can_forward()` never forwards a frame. Fix: add the interface that adds and removes the rules (and a test) in this PR, or leave the CAN part for the PR that adds it. ########## include/nuttx/net/netdev.h: ########## @@ -576,6 +576,16 @@ struct net_driver_s unsigned long arg); #endif +#ifdef CONFIG_NET_FORWARD + struct work_s d_fwdwork; /* For concurrent forwarding work */ Review Comment: Nothing uses `d_fwdwork`, but it makes every `struct net_driver_s` larger when `NET_FORWARD` is on. Fix: remove it. ########## net/netforward/Kconfig: ########## @@ -19,10 +35,10 @@ config NET_IPFORWARD_BROADCAST If selected, broadcast packets received on one network device will be forwarded though other network devices. -config NET_IPFORWARD_NSTRUCT +config NET_FORWARD_NSTRUCT Review Comment: `NET_IPFORWARD_NSTRUCT` and `NET_IPFORWARD_ALLOC_STRUCT` get new names, so a board configuration that sets the old names falls back to the defaults with no message. Fix: keep the old option names, or mark the change as breaking and say how to update a configuration. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
