On Fri, Jun 29, 2018 at 11:09:18AM -0700, Justin Pettit wrote:
> From: Andy Zhou <[email protected]>
>
> To work with kernel datapath that supports meter.
>
> Signed-off-by: Andy Zhou <[email protected]>
> Co-authored-by: Justin Pettit <[email protected]>
> Signed-off-by: Justin Pettit <[email protected]>
Thanks for reviving this.
The new code here seems to use our older declaration style where all
declarations happen at the top of functions, rather than the newer one
where they tend to be closer to initialization or first use. This is a
quibble and feel free to ignore it if you think I am nit-picking.
It's probably worth carefully looking at the log messages. Some of them
seem unnecessary, e.g. the first one in dpif_netlink_meter_transact(),
because the caller or the callee already logs. Others seem like the
wrong log level, e.g. the second one in dpif_netlink_meter_transact().
And I think that most or all should be rate-limited.
In dpif_netlink_meter_get_features(), I think that the memset() call is
not needed because dpif_meter_get_features() already does it.
In dpif_netlink_meter_set(), these checks seem like ones that should
happen centrally at a higher level:
if (!(config->flags & (OFPMF13_KBPS | OFPMF13_PKTPS))) {
return EBADF; /* Rate unit type not set. */
}
if ((config->flags & OFPMF13_KBPS) && (config->flags & OFPMF13_PKTPS)) {
return EBADF; /* Both rate units may not be set. */
}
Maybe this one too?
if (config->n_bands == 0) {
return EINVAL; /* No bands. */
}
Shouldn't dpif_netlink_meter_set() reject bands that are not "drop"?
The inner NL_NESTED_FOR_EACH in dpif_netlink_get_stats() could use
nl_attr_find().
In the case where dpif_netlink_get_stats() is used to delete a meter,
and statistics are wanted, and some bands are present but the statistics
aren't present in at least one of those bands, the function reports an
error, even though the meter was deleted successfully. That seems like
a weird corner case.
In dpif_netlink_get_stats(), 'stats' has lots of members but this
function seems to only initialize some of them. This also seems true of
dpif_meter_get().
In dpif-netlink.c, it looks to me like initialization fails if meters
aren't available. I think that breaks backward compatibility with old
kernel modules. I doubt we want that.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
