Re: [libvirt] [PATCH] support setting bandwidth from virsh attach-interface

On Thu, Oct 13, 2011 at 05:17:34PM +0800, Hong Xiang wrote: > > On 10/13/2011 02:52 PM, Hu Tao wrote: >> Adds two options, inbound and outbound, to attach-interface to set >> bandwidth when attaching interfaces >> --- >> tools/virsh.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++- >> tools/virsh.pod | 5 ++- >> 2 files changed, 98 insertions(+), 3 deletions(-) >> >> diff --git a/tools/virsh.c b/tools/virsh.c >> index bcf0603..d6c9450 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; >> >> @@ -11225,15 +11226,49 @@ 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; >> + 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) > > For input like '100,200', after this burst will point to end of > string and cause below 'if(burst)' case to fail. "if (burst&& *burst != '\0')" should fix this. But what the man page says is The strchr() and strrchr() functions return a pointer to the matched character or NULL if the character is not found Am I missing something?

> ... > virsh # attach-interface fc15 network default --inbound 100,200 > error: inbound format is incorrect > ... > >> + return -1; >> + } >> + >> + if (burst) { >> + 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; >> @@ -11254,7 +11289,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; >> } >> @@ -11270,6 +11307,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); >> >> @@ -11287,6 +11347,38 @@ 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) { >> + if (inbound.peak> 0&& inbound.burst> 0) >> + virBufferAsprintf(&buf, "<inbound average='%lld' peak='%lld' burst='%lld'/>\n", >> + inbound.average, inbound.peak, inbound.burst); >> + else if (inbound.peak> 0) >> + virBufferAsprintf(&buf, "<inbound average='%lld' peak='%lld'/>\n", >> + inbound.average, inbound.peak); >> + else if (inbound.burst> 0) >> + virBufferAsprintf(&buf, "<inbound average='%lld' burst='%lld'/>\n", >> + inbound.average, inbound.burst); >> + else >> + virBufferAsprintf(&buf, "<inbound average='%lld'/>\n", inbound.average); >> + } >> + if (outboundStr&& outbound.average> 0) { >> + if (outbound.peak> 0&& outbound.burst> 0) >> + virBufferAsprintf(&buf, "<outbound average='%lld' peak='%lld' burst='%lld'/>\n", >> + outbound.average, outbound.peak, outbound.burst); >> + else if (outbound.peak> 0) >> + virBufferAsprintf(&buf, "<outbound average='%lld' peak='%lld'/>\n", >> + outbound.average, outbound.peak); >> + else if (outbound.burst> 0) >> + virBufferAsprintf(&buf, "<outbound average='%lld' burst='%lld'/>\n", >> + outbound.average, outbound.burst); >> + else >> + virBufferAsprintf(&buf, "<outbound average='%lld'/>\n", outbound.average); >> + >> + } > > Maybe can save some typing here, :) e.g.: > ... > if (inbound.peak> 0) > virBufferAsprintf(&buf, "peak='%lld' ", inbound.peak); > ... Thanks. I'll update it. > >> + virBufferAsprintf(&buf, "</bandwidth>\n"); >> + } >> + >> virBufferAddLit(&buf, "</interface>\n"); >> >> if (virBufferError(&buf)) { >> diff --git a/tools/virsh.pod b/tools/virsh.pod >> index d0b7937..06dc06a 100644 >> --- a/tools/virsh.pod >> +++ b/tools/virsh.pod >> @@ -1235,7 +1235,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. >> @@ -1246,6 +1246,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" > > Tested-by: Hong Xiang&lt;hxiang(a)linux.vnet.ibm.com&gt; Thank you.