On Fri, Jul 3, 2026 at 1:57 AM Adrián Moreno <[email protected]> wrote:
> On Wed, Jun 17, 2026 at 12:56:04PM -0400, Mike Pattrick wrote: > > On Mon, Jun 8, 2026 at 4:55 AM Adrian Moreno via dev < > > [email protected]> wrote: > > > > > Currently, flags from an expected-to-be-local netdev are read > > > using get_flags(), which is commonly called with > > > &netdev->ifi_flags as argument. > > > > > > Refactor it into its own function and rename "update_flags" to > > > "modify_flags" to increase clarity. > > > > > > Besides, "netdev_linux_changed" does two things: updates the state of > the > > > internal cache to reflect the change _and_ actually changes the flags. > > > This strange dual-function makes many users who only want to notify a > > > change call the function with "&netdev->ifi_flags" as argument. Split > > > this behavior in two independent functions. > > > > > > As an additional benefit, this patch makes "update_flags" actually > > > report errors when the device is not accessible. > > > > > > Signed-off-by: Adrian Moreno <[email protected]> > > > --- > > > lib/netdev-linux.c | 74 +++++++++++++++++++++++++-------------- > > > tests/system-interface.at | 2 ++ > > > tests/system-tap.at | 5 ++- > > > tests/system-traffic.at | 3 +- > > > 4 files changed, 55 insertions(+), 29 deletions(-) > > > > > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > > > index 47faea8c6..eefe80865 100644 > > > --- a/lib/netdev-linux.c > > > +++ b/lib/netdev-linux.c > > > @@ -564,9 +564,13 @@ static int netdev_linux_do_ethtool(const char > *name, > > > struct ethtool_cmd *, > > > int cmd, const char *cmd_name); > > > static int get_flags(const struct netdev *, unsigned int *flags); > > > static int set_flags(const char *, unsigned int flags); > > > -static int update_flags(struct netdev_linux *netdev, enum netdev_flags > > > off, > > > +static int netdev_linux_update_via_ioctl(struct netdev_linux *); > > > +static int modify_flags(struct netdev_linux *netdev, enum netdev_flags > > > off, > > > enum netdev_flags on, enum netdev_flags > > > *old_flagsp) > > > OVS_REQUIRES(netdev->mutex); > > > +static void netdev_linux_set_flags(struct netdev_linux *netdev, > > > + unsigned int ifi_flags) > > > + OVS_REQUIRES(netdev->mutex); > > > static int get_ifindex(const struct netdev *, int *ifindexp); > > > static int do_set_addr(struct netdev *netdev, > > > int ioctl_nr, const char *ioctl_name, > > > @@ -643,7 +647,7 @@ static void netdev_linux_update(struct netdev_linux > > > *netdev, int, > > > const struct rtnetlink_change *) > > > OVS_REQUIRES(netdev->mutex); > > > static void netdev_linux_changed(struct netdev_linux *netdev, > > > - unsigned int ifi_flags, unsigned int > > > mask) > > > + unsigned int mask) > > > OVS_REQUIRES(netdev->mutex); > > > > > > /* Returns a NETLINK_ROUTE socket listening for RTNLGRP_LINK, > > > @@ -826,11 +830,10 @@ netdev_linux_run(const struct netdev_class > > > *netdev_class OVS_UNUSED) > > > SHASH_FOR_EACH (node, &device_shash) { > > > struct netdev *netdev_ = node->data; > > > struct netdev_linux *netdev = > netdev_linux_cast(netdev_); > > > - unsigned int flags; > > > > > > ovs_mutex_lock(&netdev->mutex); > > > - get_flags(netdev_, &flags); > > > - netdev_linux_changed(netdev, flags, 0); > > > + netdev_linux_update_via_ioctl(netdev); > > > + netdev_linux_changed(netdev, 0); > > > ovs_mutex_unlock(&netdev->mutex); > > > > > > netdev_close(netdev_); > > > @@ -860,17 +863,11 @@ netdev_linux_wait(const struct netdev_class > > > *netdev_class OVS_UNUSED) > > > } > > > > > > static void > > > -netdev_linux_changed(struct netdev_linux *dev, > > > - unsigned int ifi_flags, unsigned int mask) > > > +netdev_linux_changed(struct netdev_linux *dev, unsigned int mask) > > > OVS_REQUIRES(dev->mutex) > > > { > > > netdev_change_seq_changed(&dev->up); > > > > > > - if ((dev->ifi_flags ^ ifi_flags) & IFF_RUNNING) { > > > - dev->carrier_resets++; > > > - } > > > - dev->ifi_flags = ifi_flags; > > > - > > > dev->cache_valid &= mask; > > > if (!(mask & VALID_IN)) { > > > netdev_get_addrs_list_flush(); > > > @@ -885,7 +882,8 @@ netdev_linux_update__(struct netdev_linux *dev, > > > if (rtnetlink_type_is_rtnlgrp_link(change->nlmsg_type)) { > > > if (change->nlmsg_type == RTM_NEWLINK) { > > > /* Keep drv-info, ip addresses, and NUMA id. */ > > > - netdev_linux_changed(dev, change->ifi_flags, > > > + netdev_linux_set_flags(dev, change->ifi_flags); > > > + netdev_linux_changed(dev, > > > VALID_DRVINFO | VALID_IN | > > > VALID_NUMA_ID); > > > > > > /* Update netdev from rtnl-change msg. */ > > > @@ -914,13 +912,14 @@ netdev_linux_update__(struct netdev_linux *dev, > > > dev->present = true; > > > } else { > > > /* FIXME */ > > > - netdev_linux_changed(dev, change->ifi_flags, 0); > > > + netdev_linux_set_flags(dev, change->ifi_flags); > > > + netdev_linux_changed(dev, 0); > > > dev->present = false; > > > netnsid_unset(&dev->netnsid); > > > } > > > } else if (rtnetlink_type_is_rtnlgrp_addr(change->nlmsg_type)) { > > > /* Invalidates in4, in6. */ > > > - netdev_linux_changed(dev, dev->ifi_flags, ~VALID_IN); > > > + netdev_linux_changed(dev, ~VALID_IN); > > > } else { > > > OVS_NOT_REACHED(); > > > } > > > @@ -991,7 +990,7 @@ netdev_linux_construct(struct netdev *netdev_) > > > netdev_linux_set_ol(netdev_); > > > } > > > > > > - error = get_flags(&netdev->up, &netdev->ifi_flags); > > > + error = netdev_linux_update_via_ioctl(netdev); > > > if (error == ENODEV) { > > > if (netdev->up.netdev_class != &netdev_internal_class) { > > > /* The device does not exist, so don't allow it to be > opened. > > > */ > > > @@ -1038,7 +1037,7 @@ netdev_linux_construct_tap(struct netdev > *netdev_) > > > } > > > > > > /* Create tap device. */ > > > - get_flags(&netdev->up, &netdev->ifi_flags); > > > + netdev_linux_update_via_ioctl(netdev); > > > > > > if (ovsthread_once_start(&once)) { > > > if (ioctl(netdev->tap_fd, TUNGETFEATURES, &up) == -1) { > > > @@ -1907,7 +1906,7 @@ netdev_linux_set_etheraddr(struct netdev > *netdev_, > > > const struct eth_addr mac) > > > > > > /* Tap devices must be brought down before setting the address. */ > > > if (is_tap_netdev(netdev_)) { > > > - update_flags(netdev, NETDEV_UP, 0, &old_flags); > > > + modify_flags(netdev, NETDEV_UP, 0, &old_flags); > > > } > > > error = set_etheraddr(netdev_get_name(netdev_), mac); > > > if (!error || error == ENODEV) { > > > @@ -1919,7 +1918,7 @@ netdev_linux_set_etheraddr(struct netdev > *netdev_, > > > const struct eth_addr mac) > > > } > > > > > > if (is_tap_netdev(netdev_) && old_flags & NETDEV_UP) { > > > - update_flags(netdev, 0, NETDEV_UP, &old_flags); > > > + modify_flags(netdev, 0, NETDEV_UP, &old_flags); > > > } > > > > > > exit: > > > @@ -2195,7 +2194,7 @@ netdev_linux_miimon_run(void) > > > netdev_linux_get_miimon(dev->up.name, &miimon); > > > if (miimon != dev->miimon) { > > > dev->miimon = miimon; > > > - netdev_linux_changed(dev, dev->ifi_flags, 0); > > > + netdev_linux_changed(dev, 0); > > > } > > > > > > timer_set_duration(&dev->miimon_timer, > dev->miimon_interval); > > > @@ -3846,7 +3845,7 @@ iff_to_nd_flags(unsigned int iff) > > > } > > > > > > static int > > > -update_flags(struct netdev_linux *netdev, enum netdev_flags off, > > > +modify_flags(struct netdev_linux *netdev, enum netdev_flags off, > > > enum netdev_flags on, enum netdev_flags *old_flagsp) > > > OVS_REQUIRES(netdev->mutex) > > > { > > > @@ -3858,12 +3857,34 @@ update_flags(struct netdev_linux *netdev, enum > > > netdev_flags off, > > > new_flags = (old_flags & ~nd_to_iff_flags(off)) | > nd_to_iff_flags(on); > > > if (new_flags != old_flags) { > > > error = set_flags(netdev_get_name(&netdev->up), new_flags); > > > - get_flags(&netdev->up, &netdev->ifi_flags); > > > + netdev_linux_update_via_ioctl(netdev); > > > } > > > > > > return error; > > > } > > > > > > +static void > > > +netdev_linux_set_flags(struct netdev_linux *netdev, unsigned int > > > ifi_flags) > > > + OVS_REQUIRES(netdev->mutex) > > > +{ > > > + if ((netdev->ifi_flags ^ ifi_flags) & IFF_RUNNING) { > > > + netdev->carrier_resets++; > > > + } > > > + netdev->ifi_flags = ifi_flags; > > > +} > > > + > > > +static int > > > +netdev_linux_update_via_ioctl(struct netdev_linux *netdev) > > > + OVS_REQUIRES(netdev->mutex) > > > +{ > > > + unsigned int ifi_flags; > > > + int error; > > > + > > > + error = get_flags(&netdev->up, &ifi_flags); > > > + netdev_linux_set_flags(netdev, ifi_flags); > > > + return error; > > > +} > > > + > > > static int > > > netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags > off, > > > enum netdev_flags on, enum netdev_flags > > > *old_flagsp) > > > @@ -3878,14 +3899,13 @@ netdev_linux_update_flags(struct netdev > *netdev_, > > > enum netdev_flags off, > > > error = EOPNOTSUPP; > > > goto exit; > > > } > > > - error = update_flags(netdev, off, on, old_flagsp); > > > + error = modify_flags(netdev, off, on, old_flagsp); > > > } else { > > > /* Try reading flags over netlink, or fall back to ioctl. */ > > > - if (!netdev_linux_update_via_netlink(netdev)) { > > > - *old_flagsp = iff_to_nd_flags(netdev->ifi_flags); > > > - } else { > > > - error = update_flags(netdev, off, on, old_flagsp); > > > + if (netdev_linux_update_via_netlink(netdev)) { > > > + error = netdev_linux_update_via_ioctl(netdev); > > > } > > > + *old_flagsp = iff_to_nd_flags(netdev->ifi_flags); > > > } > > > > > > exit: > > > diff --git a/tests/system-interface.at b/tests/system-interface.at > > > index 20a882d1c..ea6753f30 100644 > > > --- a/tests/system-interface.at > > > +++ b/tests/system-interface.at > > > @@ -17,6 +17,7 @@ AT_CHECK([ip link add ovs-veth0 type veth peer name > > > ovs-veth1]) > > > AT_CHECK([ovs-vsctl del-port br0 ovs-veth0]) > > > > > > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > > > +/failed to get flags for network device ovs-veth0/d > > > > > > > Question, where do these log messages come from? I tried searching > through > > the source code for "failed to get flags" but couldn't find it. > > > > This error is not very grep-friendly. It is emitted by do_update_flags > (lib/netdev.c) but this since this function is (annoyingly) shared > between flag "getting" and "setting", it looks like this: > > VLOG_WARN_RL(&rl, "failed to %s flags for network device %s: %s", > off || on ? "set" : "get", netdev_get_name(netdev), > ovs_strerror(error)); > > > Before, netdev-linux was silently ignoring "get" errors, precisely > because of a confusing code sharing between flag retrieval and > modification which this patch tries to clean up. > > As the commit messages states, a by-product of the cleanup is the > correct exposure of failures. > > In this test, for example, we "ip link del" a vport. I think the correct > behavior is to bubble up the fact that netdev-linux failed to update(get) > the netdev's flags. > > Thanks. > Adrián > > Agreed. Acked-by: Mike Pattrick <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
