On Mon, Feb 16, 2015 at 10:18:32AM +0000, James Chapman wrote:
> Adding functionality to libvirt that will allow it
> query the ethtool interface for the availability
> of certain NIC HW offload features
> ---
> src/conf/device_conf.h | 6 ++
> src/conf/node_device_conf.c | 7 ++
> src/conf/node_device_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/node_device/node_device_udev.c | 4 ++
> src/util/virnetdev.c | 134 +++++++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 12 +++-
> 7 files changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 7256cdc..091f2f0 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -62,6 +62,12 @@ struct _virInterfaceLink {
> unsigned int speed; /* link speed in Mbits per second */
> };
>
> +typedef struct _virDevFeature virDevFeature;
> +typedef virDevFeature *virDevFeaturePtr;
> +struct _virDevFeature {
> + char *name; /* device feature */
> +};
> +
> int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
>
> int virDevicePCIAddressParseXML(xmlNodePtr node,
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index a728a00..7f4dbfe 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -437,6 +437,12 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
> virBufferEscapeString(&buf, "<address>%s</address>\n",
> data->net.address);
> virInterfaceLinkFormat(&buf, &data->net.lnk);
> + if (data->net.features) {
> + for (i = 0; i < data->net.nfeatures; i++) {
> + virBufferAsprintf(&buf, "<feature name='%s'/>\n",
> + data->net.features[i].name);
Storing these as an enum (for example virNodeNetDevFeature) in a
virBitmap would be nicer, in my opinion.
> + }
> + }
> if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
> const char *subtyp =
> virNodeDevNetCapTypeToString(data->net.subtype);
> @@ -1679,6 +1685,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> case VIR_NODE_DEV_CAP_NET:
> VIR_FREE(data->net.ifname);
> VIR_FREE(data->net.address);
> + VIR_FREE(data->net.features);
> break;
> case VIR_NODE_DEV_CAP_SCSI_HOST:
> VIR_FREE(data->scsi_host.wwnn);
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index fd5d179..918523a 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -141,6 +141,8 @@ struct _virNodeDevCapsDef {
> char *ifname;
> virInterfaceLink lnk;
> virNodeDevNetCapType subtype; /* LAST -> no subtype */
> + size_t nfeatures;
> + virDevFeaturePtr features;
> } net;
> struct {
> unsigned int host;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c07a561..1d165a9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1659,6 +1659,7 @@ virNetDevAddRoute;
> virNetDevClearIPAddress;
> virNetDevDelMulti;
> virNetDevExists;
> +virNetDevGetFeatures;
> virNetDevGetIndex;
> virNetDevGetIPv4Address;
> virNetDevGetLinkInfo;
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 03c7a0b..349733f 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -719,6 +719,10 @@ static int udevProcessNetworkInterface(struct udev_device *device,
> if (virNetDevGetLinkInfo(data->net.ifname, &data->net.lnk) < 0)
> goto out;
>
> + if (virNetDevGetFeatures(data->net.ifname, &data->net.features,
> + &data->net.nfeatures) < 0)
> + goto out;
> +
> ret = 0;
>
> out:
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 2a0eb43..e513c85 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -2728,3 +2728,137 @@ int virNetDevGetRxFilter(const char *ifname,
> *filter = fil;
> return ret;
> }
> +
> +#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
> +
> +/**
> + * _virNetDevFeatureAvailable
> + * This function checks for the availability of a network device feature
> + *
> + * @ifname: name of the interface
> + * @cmd: reference to an ethtool command structure
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int
> +_virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
We don't use a special prefix for internal function, we just define them
as static.
> +{
> + int ret = -1;
> + int sock = -1;
> + virIfreq ifr;
> +
> + sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
Can 'sock' be reused just like cmd?
> + if (sock < 0) {
> + virReportSystemError(errno, "%s", _("Cannot open control socket"));
> + goto cleanup;
> + }
> +
> + strcpy(ifr.ifr_name, ifname);
> + ifr.ifr_data = (void*) cmd;
> +
> + if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) {
> + /* Privileged command, no error */
> + if (errno == EPERM || errno == EINVAL) {
> + virReportSystemError(errno, "%s", _("ioctl"));
> + /* Some kernels dont support named feature, no error */
If the failure is ignored, reporting an error spams the log, a VIR_DEBUG
would be nicer.
Also, the messages aren't very descriptive.
> + } else if (errno == EOPNOTSUPP) {
> + virReportSystemError(errno, "%s", _("Warning"));
> + } else {
> + virReportSystemError(errno, "%s", _("Error"));
> + goto cleanup;
> + }
> + }
> +
> + ret = cmd->data > 0 ? 1: 0;
> + cleanup:
> + if (sock)
> + VIR_FORCE_CLOSE(sock);
> +
> + return ret;
> +}
> +
> +
> +/**
> + * virNetDevGetFeatures:
> + * This function gets the nic offloads features available for ifname
> + *
> + * @ifname: name of the interface
> + * @features: network device feature structures
> + * @nfeatures: number of features available
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int
> +virNetDevGetFeatures(const char *ifname,
> + virDevFeaturePtr *features,
> + size_t *nfeatures)
> +{
> + int ret = -1;
> + size_t i = -1;
> + size_t j = -1;
> + struct ethtool_value cmd;
> +
> + struct elem{
> + const char *name;
> + const int cmd;
> + };
> + /* legacy ethtool getters */
> + struct elem cmds[] = {
> + {"rx", ETHTOOL_GRXCSUM},
> + {"tx", ETHTOOL_GTXCSUM },
> + {"sg", ETHTOOL_GSG},
> + {"tso", ETHTOOL_GTSO},
> + {"gso", ETHTOOL_GGSO},
> + {"gro", ETHTOOL_GGRO},
> + };
> + /* ethtool masks */
> + struct elem flags[] = {
> + {"lro", ETH_FLAG_LRO},
> + {"rxvlan", ETH_FLAG_RXVLAN},
> + {"txvlan", ETH_FLAG_TXVLAN},
> + {"ntuple", ETH_FLAG_NTUPLE},
> + {"rxhash", ETH_FLAG_RXHASH},
> + };
> +
> + for (i = 0; i < (sizeof(cmds)/sizeof(struct elem)); i++) {
> + cmd.cmd = cmds[i].cmd;
> + if (_virNetDevFeatureAvailable(ifname, &cmd)) {
> + if (VIR_EXPAND_N(*features, *nfeatures, 1) < 0)
> + goto cleanup;
> + if ((ret = VIR_STRDUP((*features)[i].name, cmds[i].name)) != 1)
Here, the value of i is unrelated to the size of the allocated array.
The daemon crashes on startup on an interface that only has the "gro"
feature.
The VIR_APPEND_ELEMENT can be used here. (Or virBitmapSetBit to match
the suggestion I made above)
> + goto cleanup;
> + }
> + }
> +
> + cmd.cmd = ETHTOOL_GFLAGS;
> + for (j = 0; j < (sizeof(flags)/sizeof(struct elem)); j++) {
We have the ARRAY_CARDINALITY macro for this.
Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list