On Wed, Apr 08, 2026 at 02:45:04PM -0400, Mike Pattrick wrote:
> On Wed, Mar 11, 2026 at 2:06 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.
> >
> > 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]>
> >
>
> Overall I think this sort of refactor does make sense, comments below.
>
>
> > ---
> > lib/netdev-linux.c | 36 +++++++++++++++++++++---------------
> > tests/system-interface.at | 2 ++
> > tests/system-tap.at | 5 ++++-
> > tests/system-traffic.at | 3 ++-
> > 4 files changed, 29 insertions(+), 17 deletions(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 47faea8c6..66153bf49 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -564,7 +564,8 @@ 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 update_flags_local(struct netdev_linux *);
> >
>
> Nit but update_flags_local seems like a strange name to me. I don't know
> how others feel about it but maybe something like netdev_linux_get_flags or
> netdev_linux_update_flags might be more thematic.
>
I initially didn't add the "netdev_linux_" prefix because these are
private functions kind of paired with "get_flags", "set_flags", etc but
it's true it is more cannonical that way.
Also, I wanted to convey the fact that they are "local" as opposed to the
"standard" way of updating flags (netdev_linux_update_flags) which tries
to do it across namespaces first.
We could be explicit about the internal implementation (like the
"_via_netlink" variants) and add "_via_ioctl"). WDTY?
>
> > +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 int get_ifindex(const struct netdev *, int *ifindexp);
> > @@ -826,11 +827,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);
> > + update_flags_local(netdev);
> > + netdev_linux_changed(netdev, netdev->ifi_flags, 0);
> >
>
> Doesn't this break the carrier_resets counter? Maybe that functionality
> could be moved to the update_flags_local function.
>
Hmm... I think you're right. netdev_linux_changed currently expects
flags have not yet been updated in the netdev.
Your suggestion makes sense, I'll move the "carrier_resets" increase to
update_flags_local (or however we end up calling it).
>
> > ovs_mutex_unlock(&netdev->mutex);
> >
> > netdev_close(netdev_);
> > @@ -991,7 +991,7 @@ netdev_linux_construct(struct netdev *netdev_)
> > netdev_linux_set_ol(netdev_);
> > }
> >
> > - error = get_flags(&netdev->up, &netdev->ifi_flags);
> > + error = update_flags_local(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 +1038,7 @@ netdev_linux_construct_tap(struct netdev *netdev_)
> > }
> >
> > /* Create tap device. */
> > - get_flags(&netdev->up, &netdev->ifi_flags);
> > + update_flags_local(netdev);
> >
> > if (ovsthread_once_start(&once)) {
> > if (ioctl(netdev->tap_fd, TUNGETFEATURES, &up) == -1) {
> > @@ -1907,7 +1907,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 +1919,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:
> > @@ -3846,7 +3846,14 @@ iff_to_nd_flags(unsigned int iff)
> > }
> >
> > static int
> > -update_flags(struct netdev_linux *netdev, enum netdev_flags off,
> > +update_flags_local(struct netdev_linux *netdev)
> > + OVS_REQUIRES(netdev->mutex)
> > +{
> > + return get_flags(&netdev->up, &netdev->ifi_flags);
> > +}
> > +
> > +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)
> > {
> > @@ -3858,7 +3865,7 @@ 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);
> > + update_flags_local(netdev);
> > }
> >
> > return error;
> > @@ -3878,14 +3885,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 = update_flags_local(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
> > /could not open network device ovs-veth0/d
> > /cannot get .*STP status on nonexistent port/d
> > /ethtool command .*on network device ovs-veth0 failed/d
> > @@ -177,6 +178,7 @@ AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl
> >
> > OVS_TRAFFIC_VSWITCHD_STOP(["
> > /could not open network device ovs-veth0 (No such device)/d
> > + /failed to get flags for network device ovs-veth0: No such device/d
> > "])
> > AT_CLEANUP
> >
> > diff --git a/tests/system-tap.at b/tests/system-tap.at
> > index 03ec01270..fe237ad10 100644
> > --- a/tests/system-tap.at
> > +++ b/tests/system-tap.at
> > @@ -29,6 +29,9 @@ NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2
> > 10.1.1.2 | FORMAT_PING], [0],
> > OVS_START_L7([at_ns1], [http])
> > NS_CHECK_EXEC([at_ns0], OVS_GET_HTTP([10.1.1.2]), [0], [ignore], [ignore])
> >
> > -OVS_TRAFFIC_VSWITCHD_STOP(["/.*ethtool command ETHTOOL_G.*/d"])
> > +OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> > +/.*ethtool command ETHTOOL_G.*/d
> > +/.*failed to get flags for network device/d
> > +"])
> >
> > AT_CLEANUP
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 8f4fdf8b1..007b00859 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -4297,7 +4297,8 @@
> > tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
> >
> > OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> > /ioctl(SIOCGIFINDEX) on .* device failed: No such device/d
> > -/removing policing failed: No such device/d"])
> > +/removing policing failed: No such device/d
> > +/failed to get flags for network device/d"])
> > AT_CLEANUP
> >
> > AT_SETUP([conntrack - ct_mark])
> > --
> > 2.53.0
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
