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

> +/* 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 (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.

ACK with those nits fixed.