On 3/28/25 10:54 AM, Wilson Peng via dev wrote:
> This fix could solve the issue for not updated ifi_flags and mac
> of internal type port which may could not get the correct ifi_flags and
> mac info when the internal type port is created on Windows platform.
>
> With the patch, if user is removing some network adapter via CMD below,
> OVS on Windows could also update the value of fields admin_state and
> link_state as "down". Related MAC is also set zero.
>
> Remove-VMNetworkAdapter -ManagementOS -SwitchName br-int
> -VMNetworkAdapterName xxx
>
> As in ovs-windows it is not set ifindex on ovs interface, it may need
> another patch to address the not-implemented ifIndex which could be
> got via CMD ovs-vsctl list interface.
>
> Reported-at:https://github.com/openvswitch/ovs-issues/issues/351
> Reported-at:https://github.com/openvswitch/ovs-issues/issues/353
>
> Signed-off-by: Wilson Peng <wilson.p...@broadcom.com>
> ---
> lib/dpif-netlink.c | 4 ++
> lib/netdev-windows.c | 107 +++++++++++++++++++++++++++++++++++++------
> 2 files changed, 96 insertions(+), 15 deletions(-)
Hi, Wilson Peng. Thanks for the patch!
I haven't tested this, but see some comments below.
Best regards, Ilya Maximets.
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index f8850181d..ef5254e36 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -751,6 +751,10 @@ dpif_netlink_run(struct dpif *dpif_)
> dpif_netlink_refresh_handlers_vport_dispatch(dpif,
> dpif->n_handlers);
> fat_rwlock_unlock(&dpif->upcall_lock);
> + #ifdef _WIN32
The ifdef/endif should be at the beginning of the line and the lines below
should be indented the same as the other code above.
> + seq_change(connectivity_seq_get());
> + VLOG_INFO("Do seq_change for connectivity_seq");
This is a very internal thing, it should not be in the log. I'd suggest to
add a comment in the code instead explaining why we do that.
> + #endif
> }
> }
> return false;
> diff --git a/lib/netdev-windows.c b/lib/netdev-windows.c
> index 3fad501e3..6e2922584 100644
> --- a/lib/netdev-windows.c
> +++ b/lib/netdev-windows.c
> @@ -173,7 +173,7 @@ netdev_windows_system_construct(struct netdev *netdev_)
> type = netdev_get_type(&netdev->up);
> if (type && !strcmp(type, "system") &&
> (info.ovs_type == OVS_VPORT_TYPE_INTERNAL)) {
> - VLOG_DBG("construct device %s, ovs_type: %u failed",
> + VLOG_ERR("construct device %s, ovs_type: %u failed",
Errors are normally for internal errors, and it seems like this one can
happen under normal circumstances. It should be a warning instead.
> netdev_get_name(&netdev->up), info.ovs_type);
> return 1;
> }
> @@ -183,15 +183,18 @@ netdev_windows_system_construct(struct netdev *netdev_)
> netdev->port_no = info.port_no;
>
> netdev->mac = info.mac_address;
> - netdev->cache_valid = VALID_ETHERADDR;
> + if (!eth_addr_is_zero(netdev->mac)) {
> + netdev->cache_valid = VALID_ETHERADDR;
> + }
Should we be cautious and set the flag to zero before checking?
The caller uses memset to clear the whole structure, but it doesn't
seem right to rely on that.
> netdev->ifindex = -EOPNOTSUPP;
>
> netdev->mtu = info.mtu;
> netdev->cache_valid |= VALID_MTU;
>
> netdev->ifi_flags = dp_to_netdev_ifi_flags(info.ifi_flags);
> - netdev->cache_valid |= VALID_IFFLAG;
> -
> + if (netdev->ifi_flags) {
> + netdev->cache_valid |= VALID_IFFLAG;
> + }
> VLOG_DBG("construct device %s, ovs_type: %u.",
> netdev_get_name(&netdev->up), info.ovs_type);
> return 0;
> @@ -330,14 +333,40 @@ netdev_windows_get_etheraddr(const struct netdev
> *netdev_,
> struct eth_addr *mac)
> {
> struct netdev_windows *netdev = netdev_windows_cast(netdev_);
> + struct netdev_windows_netdev_info info;
> + struct ofpbuf *buf = NULL;
> + const char *type = NULL;
> + const char *dev_name = NULL;
> + int ret;
>
> - ovs_assert((netdev->cache_valid & VALID_ETHERADDR) != 0);
> if (netdev->cache_valid & VALID_ETHERADDR) {
> *mac = netdev->mac;
> - } else {
> - return EINVAL;
> + return 0;
> + } else if (eth_addr_is_zero(netdev->mac)) {
> + type = netdev_get_type(&netdev->up);
> + if (type && !strcmp(type, "internal")) {
Do we need to check the type here? Shouldn't this be executed for
all types?
> + dev_name = netdev_get_name(&netdev->up);
> + if (dev_name) {
No need to check for the name here, it must be set. In a worst case,
an assertion can be used.
> + ret = query_netdev(dev_name, &info, &buf);
> + if (!ret) {
> + ofpbuf_delete(buf);
> + *mac = info.mac_address;
> + if (!eth_addr_is_zero(info.mac_address)) {
Do we need this check? Some devices may legitimately have zero
MAC addresses for some time. If the query didn't fail, we should
probably just cache the result.
> + netdev->mac = info.mac_address;
> + netdev->cache_valid |= VALID_ETHERADDR;
> + }
> + VLOG_DBG("get_etheraddr query_netdev dev %s success",
> + dev_name);
> + return 0;
> + } else {
> + VLOG_ERR("get_etheraddr query_netdev dev %s failed",
> + dev_name);
Should be a warning.
> + *mac = eth_addr_zero;
> + }
> + }
> + }
> }
> - return 0;
> + return EINVAL;
> }
>
> static int
> @@ -363,8 +392,6 @@ netdev_windows_set_etheraddr(const struct netdev *netdev_,
> return 0;
> }
>
> -/* This functionality is not really required by the datapath.
> - * But vswitchd bringup expects this to be implemented. */
> static int
> netdev_windows_update_flags(struct netdev *netdev_,
> enum netdev_flags off,
> @@ -372,15 +399,65 @@ netdev_windows_update_flags(struct netdev *netdev_,
> enum netdev_flags *old_flagsp)
> {
> struct netdev_windows *netdev = netdev_windows_cast(netdev_);
> + struct netdev_windows_netdev_info info;
> + struct ofpbuf *buf = NULL;
> + const char *type = NULL;
> + const char *dev_name = NULL;
> + uint32_t ifi_flags = 0;
> + int ret;
> +
> + type = netdev_get_type(&netdev->up);
> + if (type && !strcmp(type, "internal")) {
> + dev_name = netdev_get_name(&netdev->up);
> + if (dev_name) {
> + ret = query_netdev(dev_name, &info, &buf);
> + if (!ret) {
> + ofpbuf_delete(buf);
> + ifi_flags = dp_to_netdev_ifi_flags(info.ifi_flags);
> + if (ifi_flags) {
> + *old_flagsp = ifi_flags;
> + netdev->ifi_flags = ifi_flags;
> + netdev->cache_valid |= VALID_IFFLAG;
> + }
> +
> + if (eth_addr_is_zero(netdev->mac) &&
> + !eth_addr_is_zero(info.mac_address)) {
> + netdev->mac = info.mac_address;
> + netdev->cache_valid |= VALID_ETHERADDR;
> + }
> + VLOG_DBG("update_flags query_netdev dev %s success: %d",
> + dev_name, *old_flagsp);
> + return 0;
We're doing the same thing here and in the netdev_windows_get_etheraddr()
function. Please, create a new function that updates both the flags and
the address in the cache, and then call it from both places in case the
cache is not already valid and then return the appropriate cached value.
Use netdev_linux_update__()/netdev_linux_get_etheraddr() as an example.
> + } else {
> + VLOG_ERR("update_flags query_netdev dev %s failed",
> + dev_name);
> + *old_flagsp = 0;
> + netdev->ifi_flags = 0;
> + if (netdev->cache_valid & VALID_IFFLAG) {
> + netdev->cache_valid &= ~VALID_IFFLAG;
> + /* On ofproto_run() it will check port status
> + * for any port whose netdev has changed
> + * Here netdev_change_seq_changed would force do
> + * update_port()
> + */
> + netdev_change_seq_changed(&netdev->up);
> + VLOG_INFO("update_flags failed; update_port for %s",
> + dev_name);
> + }
> + if (!eth_addr_is_zero(netdev->mac)) {
> + netdev->mac = eth_addr_zero;
> + netdev->cache_valid &= ~VALID_ETHERADDR;
> + }
> + return 0;
> + }
> + }
> + }
>
> - ovs_assert((netdev->cache_valid & VALID_IFFLAG) != 0);
> if (netdev->cache_valid & VALID_IFFLAG) {
> *old_flagsp = netdev->ifi_flags;
> - /* Setting the interface flags is not supported. */
> - } else {
> - return EINVAL;
> + return 0;
> }
> - return 0;
> + return EINVAL;
> }
>
> /* Looks up in the ARP table entry for a given 'ip'. If it is found, the
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
