[libvirt] [PATCH v3 2/3] network: allow to specify timeout for openvswitch calls

[Michal Privoznik]

On 02/07/2017 04:16 PM, Boris Fiuczynski wrote:
> This patchs allows to set the timeout value used for all
> openvswitch calls. The default timeout value remains as
> before at 5 seconds.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> ---
>  src/libvirt_private.syms        |  1 +
>  src/util/virnetdevopenvswitch.c | 64 +++++++++++++++++++++++++++++++++++------
>  src/util/virnetdevopenvswitch.h |  4 +++
>  3 files changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d556c7d..0a7de9a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2078,6 +2078,7 @@ virNetDevOpenvswitchGetVhostuserIfname;
>  virNetDevOpenvswitchInterfaceStats;
>  virNetDevOpenvswitchRemovePort;
>  virNetDevOpenvswitchSetMigrateData;
> +virNetDevOpenvswitchSetTimeout;
>  
>  
>  # util/virnetdevtap.h
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index e6cb096..3a11fb4 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (C) 2013 Red Hat, Inc.
>   * Copyright (C) 2012 Nicira, Inc.
> + * Copyright (C) 2017 IBM Corporation
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -20,6 +21,7 @@
>   *     Dan Wendlandt <dan at nicira.com>
>   *     Kyle Mestery <kmestery at cisco.com>
>   *     Ansis Atteka <aatteka at nicira.com>
> + *     Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>   */
>  
>  #include <config.h>
> @@ -38,6 +40,23 @@
>  
>  VIR_LOG_INIT("util.netdevopenvswitch");
>  
> +/*
> + * Set openvswitch default timout
> + */
> +static unsigned int virNetDevOpenvswitchTimeout = VIR_NETDEV_OVS_DEFAULT_TIMEOUT;

Ah, yet another global variable. But I guess there is no other way how to achieve this since we don't have a struct where we'd keep internal state.

> +
> +/**
> + * virNetDevOpenvswitchSetTimeout:
> + * @timeout: the timeout in seconds
> + *
> + * Set the openvswitch timeout
> + */
> +void
> +virNetDevOpenvswitchSetTimeout(unsigned int timeout)
> +{
> +    virNetDevOpenvswitchTimeout = timeout;
> +}
> +
>  /**
>   * virNetDevOpenvswitchAddPort:
>   * @brname: the bridge name
> @@ -66,6 +85,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>      char *ifaceid_ex_id = NULL;
>      char *profile_ex_id = NULL;
>      char *vmid_ex_id = NULL;
> +    char *ovs_timeout = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
>      virMacAddrFormat(macaddr, macaddrstr);
> @@ -86,10 +106,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>                          ovsport->profileID) < 0)
>              goto cleanup;
>      }
> +    if (virAsprintf(&ovs_timeout, "--timeout=%d", virNetDevOpenvswitchTimeout) < 0)
> +        goto cleanup;
>  
>      cmd = virCommandNew(OVSVSCTL);
>  
> -    virCommandAddArgList(cmd, "--timeout=5", "--", "--if-exists", "del-port",
> +    virCommandAddArgList(cmd, ovs_timeout, "--", "--if-exists", "del-port",
>                           ifname, "--", "add-port", brname, ifname, NULL);

While this would work we have virCommandAddArgFormat which wraps exactly this:
virCommandNew(OVSVSCTL);
virCommandAddArgFormat(cmd, "--timeout=%u", virNetDevOpenvswitchTimeout);
virCommandAddArgList(cmd, "--", "--if-exists", ..., NULL);

Then we can take the extra step and wrap it in a static function so that --timeout=%u doesn't have to be copied all over the place.

I will fix this before pushing.

ACK with the change I'm suggesting.

Michal