On Tue, Aug 26, 2025 at 7:19 AM David Marchand <david.march...@redhat.com>
wrote:
> On Mon, 25 Aug 2025 at 21:58, Mike Pattrick <m...@redhat.com> wrote:
> >> @@ -1145,53 +1141,82 @@ netdev_bsd_get_features(const struct netdev
> *netdev,
> >> error = af_inet_ioctl(SIOCGIFMEDIA, &ifmr);
> >> if (error) {
> >> VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
> >> - netdev_get_name(netdev), ovs_strerror(error));
> >> + netdev_get_name(&netdev->up), ovs_strerror(error));
> >> goto cleanup;
> >> }
> >>
> >> /* Current settings. */
> >> - *current = netdev_bsd_parse_media(ifmr.ifm_active);
> >> - if (!*current) {
> >> - *current = NETDEV_F_OTHER;
> >> + netdev->current = netdev_bsd_parse_media(ifmr.ifm_active);
> >> + if (!netdev->current) {
> >> + netdev->current = NETDEV_F_OTHER;
> >> }
> >>
> >> /* Advertised features. */
> >> - *advertised = netdev_bsd_parse_media(ifmr.ifm_current);
> >> + netdev->advertised = netdev_bsd_parse_media(ifmr.ifm_current);
> >>
> >> /* Supported features. */
> >> - *supported = 0;
> >> + netdev->supported = 0;
> >> for (i = 0; i < ifmr.ifm_count; i++) {
> >> - *supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]);
> >> + netdev->supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]);
> >> }
> >>
> >> - /* Peer advertisements. */
> >> - *peer = 0; /* XXX */
> >> -
> >> error = 0;
> >> +
> >> cleanup:
> >> free(media_list);
> >> + netdev->get_features_error = error;
> >
> >
> > Why not just return this value?
>
> I copied this logic from the linux netdev.
> I am not clear about the original intent.
>
> At the moment, it makes the get_features_error and other (current,
> advertised etc..) fields all populated by this same helper in a
> unified way (retrieved values are all stored in the netdev specific
> object).
>
> I don't mind changing this here if there is strong opinion against.
>
netdev-linux also uses a "cache_valid & VALID_FEATURES" mechanism, and
stores the error/results so ETHTOOL_GSET can be called only once. But that
doesn't appear to be used here, so the use of this cache isn't quite clear
to me.
I think it would make sense to either not cache the values at all, or have
some way to avoid or return early from the netdev_bsd_read_features call,
like with netdev-linux.
-M
>
>
> --
> David Marchand
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
