[libvirt] [PATCH v2] support setting bandwidth from virsh attach-interface(was Re: [PATCH] support setting bandwidth from virsh attach-interface)

[Michal Privoznik]

On 14.10.2011 04:49, Hu Tao wrote:
> Here is v2:
> 
>>From 3c5885380ffd68769f2c7d82eb21bd7aca49393e Mon Sep 17 00:00:00 2001
> From: Hu Tao <hutao at cn.fujitsu.com>
> Date: Fri, 14 Oct 2011 09:10:39 +0800
> Subject: [PATCH v2] support setting bandwidth from virsh attach-interface
> 
> Adds two options, inbound and outbound, to attach-interface to set
> bandwidth when attaching interfaces
> ---
>  tools/virsh.c   |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  tools/virsh.pod |    5 ++-
>  2 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 54684f6..ce40a57 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -59,6 +59,7 @@
>  #include "threads.h"
>  #include "command.h"
>  #include "virkeycode.h"
> +#include "network.h"
>  
>  static char *progname;
>  
> @@ -11228,15 +11229,51 @@ static const vshCmdOptDef opts_attach_interface[] = {
>      {"script", VSH_OT_DATA, 0, N_("script used to bridge network interface")},
>      {"model", VSH_OT_DATA, 0, N_("model type")},
>      {"persistent", VSH_OT_BOOL, 0, N_("persist interface attachment")},
> +    {"inbound", VSH_OT_DATA, VSH_OFLAG_NONE, N_("control domain's incoming traffics")},
> +    {"outbound", VSH_OT_DATA, VSH_OFLAG_NONE, N_("control domain's outgoing traffics")},
>      {NULL, 0, 0, NULL}
>  };
>  
> +/* parse inbound and outbound which are in the format of
> + * 'average,peak,burst', in which peak and burst are optional,
> + * thus 'average,,burst' and 'average,peak' are also legal. */
> +static int parseRateStr(const char *rateStr, virRatePtr rate)
> +{
> +    char *average = NULL, *peak = NULL, *burst = NULL;
> +
> +    average = (char *)rateStr;

I'd vote for const correctness here;
So, either change average to be const char *, or leave it out and
use passed rateStr directly instead.

> +    if (!average)
> +        return -1;
> +    rate->average = atol(average);
> +
> +    peak = strchr(average, ',');
> +    if (peak) {
> +        burst = strchr(peak + 1, ',');
> +        if (!(burst && (burst - peak == 1))) {
> +            if (virStrToLong_ull(peak + 1, &burst, 10, &rate->peak) < 0)
> +                return -1;
> +        }
> +
> +        /* burst will be updated to point to the end of rateStr in case
> +         * of 'average,peak' */
> +        if (burst && *burst != '\0') {
> +            if (virStrToLong_ull(burst + 1, NULL, 10, &rate->burst) < 0)
> +                return -1;
> +        }
> +    }
> +
> +
> +    return 0;
> +}
> +
>  static bool
>  cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom = NULL;
>      const char *mac = NULL, *target = NULL, *script = NULL,
> -                *type = NULL, *source = NULL, *model = NULL;
> +                *type = NULL, *source = NULL, *model = NULL,
> +                *inboundStr = NULL, *outboundStr = NULL;
> +    virRate inbound, outbound;
>      int typ;
>      int ret;
>      bool functionReturn = false;
> @@ -11257,7 +11294,9 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>          vshCommandOptString(cmd, "target", &target) < 0 ||
>          vshCommandOptString(cmd, "mac", &mac) < 0 ||
>          vshCommandOptString(cmd, "script", &script) < 0 ||
> -        vshCommandOptString(cmd, "model", &model) < 0) {
> +        vshCommandOptString(cmd, "model", &model) < 0 ||
> +        vshCommandOptString(cmd, "inbound", &inboundStr) < 0 ||
> +        vshCommandOptString(cmd, "outbound", &outboundStr) < 0) {
>          vshError(ctl, "missing argument");
>          goto cleanup;
>      }
> @@ -11273,6 +11312,29 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>          goto cleanup;
>      }
>  
> +    if (inboundStr) {
> +        memset(&inbound, 0, sizeof(inbound));
> +        if (parseRateStr(inboundStr, &inbound) < 0) {
> +            vshError(ctl, _("inbound format is incorrect"));
> +            goto cleanup;
> +        }
> +        if (inbound.average == 0) {
> +            vshError(ctl, _("inbound average is mandatory"));
> +            goto cleanup;
> +        }
> +    }
> +    if (outboundStr) {
> +        memset(&outbound, 0, sizeof(outbound));
> +        if (parseRateStr(outboundStr, &outbound) < 0) {
> +            vshError(ctl, _("outbound format is incorrect"));
> +            goto cleanup;
> +        }
> +        if (outbound.average == 0) {
> +            vshError(ctl, _("outbound average is mandatory"));
> +            goto cleanup;
> +        }
> +    }
> +
>      /* Make XML of interface */
>      virBufferAsprintf(&buf, "<interface type='%s'>\n", type);
>  
> @@ -11290,6 +11352,27 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>      if (model != NULL)
>          virBufferAsprintf(&buf, "  <model type='%s'/>\n", model);
>  
> +    if (inboundStr || outboundStr) {
> +        virBufferAsprintf(&buf, "  <bandwidth>\n");
> +        if (inboundStr && inbound.average > 0) {
> +            virBufferAsprintf(&buf, "    <inbound average='%lld'", inbound.average);
> +            if (inbound.peak > 0)
> +                virBufferAsprintf(&buf, " peak='%lld'", inbound.peak);
> +            if (inbound.burst > 0)
> +                virBufferAsprintf(&buf, " burst='%lld'", inbound.burst);
> +            virBufferAsprintf(&buf, "/>\n");
> +        }
> +        if (outboundStr && outbound.average > 0) {
> +            virBufferAsprintf(&buf, "    <outbound average='%lld'", outbound.average);
> +            if (outbound.peak > 0)
> +                virBufferAsprintf(&buf, " peak='%lld'", outbound.peak);
> +            if (outbound.burst > 0)
> +                virBufferAsprintf(&buf, " burst='%lld'", outbound.burst);
> +            virBufferAsprintf(&buf, "/>\n");
> +        }
> +        virBufferAsprintf(&buf, "  </bandwidth>\n");
> +    }
> +

Since [in|out]bound average, peak and burst are defined as unsigned long
long, you can actually check for (outbound.peak) instead of
(outbound.peak > 0), but you can leave it as-is. But I'd prefer to
change print format from '%lld' to '%llu'. I am surprised my gcc does
not warn about it.

>      virBufferAddLit(&buf, "</interface>\n");
>  
>      if (virBufferError(&buf)) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 74ae647..43a4f4c 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1237,7 +1237,7 @@ scsi:controller.bus.unit or ide:controller.bus.unit.
>  
>  =item B<attach-interface> I<domain-id> I<type> I<source>
>  [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>]
> -[I<--persistent>]
> +[I<--persistent>] [I<--inbound average,peak,burst>] [I<--outbound average,peak,burst>]
>  
>  Attach a new network interface to the domain.
>  I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device.
> @@ -1248,6 +1248,9 @@ I<script> allows to specify a path to a script handling a bridge instead of
>  the default one.
>  I<model> allows to specify the model type.
>  I<persistent> indicates the changes will affect the next boot of the domain.
> +I<inbound> and I<outbound> control the bandwidth of the interface. I<peak>
> +and I<burst> are optional, so "average,peak", "average,,burst" and
> +"average" are also legal.
>  
>  B<Note>: the optional target value is the name of a device to be created
>  as the back-end on the node. If not provided a device named "vnetN" or "vifN"

ACK with those nits fixed.

Michal