[libvirt] [PATCH v2] support setting bandwidth from virsh attach-interface(was Re: [PATCH] support setting bandwidth from virsh attach-interface)
Hong Xiang
hxiang at linux.vnet.ibm.com
Fri Oct 14 03:01:07 UTC 2011
On 10/14/2011 10:49 AM, Hu Tao wrote:
>>>>> +/* 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?
>>
>> It's 'virStrToLong_ull(peak + 1,&burst, 10,&rate->peak)' that will
>
> You're right.
>
>> overwrite burst, maybe you can pass a NULL instead of '&burst' to
>> this call, and leave below 'if(burst)' as is.
>
> Passing a NULL fails virStrToLong_ull in case of '100,200,300'.
Oh, I thought virStrToLong_ull() accepts a NULL endptr and I was wrong.
>
> 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;
> + 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");
> + }
> +
> 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"
I'm ok with this new patch, now sure if I can ACK or not though, :)
--
Thanks.
Hong Xiang
More information about the libvir-list
mailing list