[libvirt] [PATCH 2/2] virsh: Properly reject 'floor' settings in cmdAttachInterface

[Peter Krempa]

On Wed, Aug 12, 2015 at 09:59:08 +0200, Michal Privoznik wrote:
> On 12.08.2015 07:45, Peter Krempa wrote:
> > cmdAttachInterface doesn't support the 'floor' field that was added in
> > d7f5c88961b52 but that commit didn't properly reject it from
> > cmdAttachInterface where it's unused.
> > ---
> >  tools/virsh-domain.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 9f54691..94e7a5c 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -963,8 +963,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> >          memset(&inbound, 0, sizeof(inbound));
> >          if (vshParseRateStr(ctl, inboundStr, &inbound) < 0)
> >              goto cleanup;
> > -        if (!inbound.average && !inbound.floor) {
> > -            vshError(ctl, _("either inbound average or floor is mandatory"));
> > +        if (!inbound.average) {
> > +            vshError(ctl, _("inbound average is mandatory"));
> > +            goto cleanup;
> > +        }
> > +        if (inbound.floor) {
> > +            vshError(ctl, _("inbound floor is unsupported yet"));
> >              goto cleanup;
> >          }
> >      }
> > 
> 
> Ah, it isn't. But it's not limitation of our API (which does support @floor of course). It's a virsh limitation. So how about instead of you patch have the following in?
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a957836..7cd521e 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1041,12 +1041,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>      if (inboundStr || outboundStr) {
>          virBufferAddLit(&buf, "<bandwidth>\n");
>          virBufferAdjustIndent(&buf, 2);
> -        if (inboundStr && inbound.average > 0) {
> -            virBufferAsprintf(&buf, "<inbound average='%llu'", inbound.average);
> +        if (inboundStr && (inbound.average || inbound.floor)) {
> +            virBufferAddLit(&buf, "<inbound");
> +            if (inbound.average > 0)
> +                virBufferAsprintf(&buf, " average='%llu'", inbound.average);
>              if (inbound.peak > 0)
>                  virBufferAsprintf(&buf, " peak='%llu'", inbound.peak);
>              if (inbound.burst > 0)
>                  virBufferAsprintf(&buf, " burst='%llu'", inbound.burst);
> +            if (inbound.floor > 0)
> +                virBufferAsprintf(&buf, " floor='%llu'", inbound.floor);
>              virBufferAddLit(&buf, "/>\n");
>          }
>          if (outboundStr && outbound.average > 0) {

Feel free to do it that way. I've noticed that the check didn't make
much sense as it was now and I didn't care enough to see whether it was
possible to use that.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150812/e2ab7b7e/attachment-0001.sig>