[libvirt] [PATCH 5/5] Reuse the socket in virNetDevGetFeatures

[Peter Krempa]

On Mon, Jun 06, 2016 at 09:39:28 +0200, Ján Tomko wrote:
> This speeds up node_device_udev driver startup 11x.
> ---
>  src/util/virnetdev.c | 69 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 52f02d3..7863e8a 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -3206,20 +3206,11 @@ virNetDevRDMAFeature(const char *ifname,
>   * Returns 0 on success, -1 on failure.
>   */
>  static int
> -virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
> +virNetDevSendEthtoolIoctl(int fd, struct ifreq *ifr)

You changed arguments but didn't fix the comment that documents them.

>  {
>      int ret = -1;
> -    int fd = -1;
> -    struct ifreq ifr;
> -
> -    /* Ultimately uses AF_PACKET for socket which requires privileged
> -     * daemon support.
> -     */
> -    if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
> -        return ret;
>  
> -    ifr.ifr_data = cmd;
> -    ret = ioctl(fd, SIOCETHTOOL, &ifr);
> +    ret = ioctl(fd, SIOCETHTOOL, ifr);
>      if (ret != 0) {
>          switch (errno) {
>              case EINVAL: /* kernel doesn't support SIOCETHTOOL */
> @@ -3230,12 +3221,10 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
>                  break;
>              default:
>                  virReportSystemError(errno, "%s", _("ethtool ioctl error"));
> -                goto cleanup;
> +                break;

These error reports don't make much sense now, but I guess we can keep
them for debugging purposes.

>          }
>      }
>  
> - cleanup:
> -    VIR_FORCE_CLOSE(fd);
>      return ret;
>  }
>  
> @@ -3255,10 +3244,10 @@ struct ethtool_to_virnetdev_feature {
>  * Returns 0 if not found, 1 on success, and -1 on failure.

Again. Argument change is not documented.

>  */
>  static bool
> -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
> +virNetDevFeatureAvailable(int fd, struct ifreq *ifr, struct ethtool_value *cmd)
>  {
> -    cmd = (void*)cmd;
> -    if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0 &&
> +    ifr->ifr_data = (void*)cmd;
> +    if (virNetDevSendEthtoolIoctl(fd, ifr) == 0 &&
>          cmd->data > 0)
>          return true;
>      return false;


> @@ -3332,10 +3322,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
>   * Returns 0 if not found, 1 on success, and -1 on failure.

And here too ...

>   */
>  static bool
> -virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
> +virNetDevGFeatureAvailable(int fd,
> +                           struct ifreq *ifr,
> +                           struct ethtool_gfeatures *cmd)
>  {
> -    cmd = (void*)cmd;
> -    if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0)
> +    ifr->ifr_data = (void*)cmd;
> +    if (virNetDevSendEthtoolIoctl(fd, ifr) == 0)
>          return !!FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
>      return false;
>  }
> @@ -3343,7 +3335,8 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
>  
>  static int
>  virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
> -                             const char *ifname)
> +                            int fd,
> +                            struct ifreq *ifr)

Broken formatting.

>  {
>      struct ethtool_gfeatures *g_cmd;
>  

[...]

> @@ -3391,14 +3389,23 @@ virNetDevGetFeatures(const char *ifname,
>          return 0;
>      }
>  
> -    virNetDevGetEthtoolFeatures(*out, ifname);
> +    /* Ultimately uses AF_PACKET for socket which requires privileged
> +     * daemon support.
> +     */
> +    if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
> +        goto cleanup;
>  
> -    if (virNetDevGetEthtoolGFeatures(*out, ifname) < 0)
> -        return -1;
> +    virNetDevGetEthtoolFeatures(*out, fd, &ifr);
> +
> +    if (virNetDevGetEthtoolGFeatures(*out, fd, &ifr) < 0)
> +        goto cleanup;
>  
>      if (virNetDevRDMAFeature(ifname, out) < 0)
> -        return -1;
> -    return 0;
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:

You forgot to close the socket.

> +    return ret;
>  }
>  #else
>  int

ACK with the issues fixed.